diff options
8 files changed, 75 insertions, 78 deletions
diff --git a/configserver-client/pom.xml b/configserver-client/pom.xml index cee6f6c067f..0a29ba003f4 100644 --- a/configserver-client/pom.xml +++ b/configserver-client/pom.xml @@ -55,13 +55,7 @@ <dependency> <groupId>org.junit.jupiter</groupId> - <artifactId>junit-jupiter-engine</artifactId> - <scope>test</scope> - </dependency> - <dependency> - <groupId>com.yahoo.vespa</groupId> - <artifactId>testutil</artifactId> - <version>${project.version}</version> + <artifactId>junit-jupiter</artifactId> <scope>test</scope> </dependency> <dependency> diff --git a/configserver-client/src/main/java/ai/vespa/hosted/client/AbstractConfigServerClient.java b/configserver-client/src/main/java/ai/vespa/hosted/client/AbstractConfigServerClient.java index 0ee9e320259..e11f0db7194 100644 --- a/configserver-client/src/main/java/ai/vespa/hosted/client/AbstractConfigServerClient.java +++ b/configserver-client/src/main/java/ai/vespa/hosted/client/AbstractConfigServerClient.java @@ -52,7 +52,7 @@ public abstract class AbstractConfigServerClient implements ConfigServerClient { protected abstract ClassicHttpResponse execute(ClassicHttpRequest request, HttpClientContext context) throws IOException; /** Executes the given request with response/error handling and retries. */ - private <T> T execute(RequestBuilder builder, BiFunction<ClassicHttpResponse, IOException, T> handler) { + private <T> T execute(RequestBuilder builder, Function<ClassicHttpResponse, T> handler, Function<IOException, T> catcher) { HttpClientContext context = HttpClientContext.create(); context.setRequestConfig(builder.config); @@ -62,10 +62,10 @@ public abstract class AbstractConfigServerClient implements ConfigServerClient { request.setEntity(builder.entity); try { try { - return handler.apply(execute(request, context), null); + return handler.apply(execute(request, context)); } catch (IOException e) { - return handler.apply(null, e); + return catcher.apply(e); } } catch (RetryException e) { @@ -133,7 +133,7 @@ public abstract class AbstractConfigServerClient implements ConfigServerClient { } @Override - public RequestBuilder at(String... pathSegments) { + public RequestBuilder at(List<String> pathSegments) { uriBuilder.setPathSegments(requireNonNull(pathSegments)); return this; } @@ -150,19 +150,19 @@ public abstract class AbstractConfigServerClient implements ConfigServerClient { } @Override - public RequestBuilder parameters(String... pairs) { - if (pairs.length % 2 != 0) + public RequestBuilder parameters(List<String> pairs) { + if (pairs.size() % 2 != 0) throw new IllegalArgumentException("Must supply parameter key/values in pairs"); - for (int i = 0; i < pairs.length; ) - uriBuilder.setParameter(pairs[i++], pairs[i++]); + for (int i = 0; i < pairs.size(); ) + uriBuilder.setParameter(pairs.get(i++), pairs.get(i++)); return this; } @Override public RequestBuilder timeout(Duration timeout) { - return config(RequestConfig.copy(defaultRequestConfig) + return config(RequestConfig.copy(config) .setResponseTimeout(timeout.toMillis(), TimeUnit.MILLISECONDS) .build()); } @@ -175,8 +175,8 @@ public abstract class AbstractConfigServerClient implements ConfigServerClient { } @Override - public <T> T handle(BiFunction<ClassicHttpResponse, IOException, T> handler) throws UncheckedIOException { - return execute(this, requireNonNull(handler)); + public <T> T handle(Function<ClassicHttpResponse, T> handler, Function<IOException, T> catcher) throws UncheckedIOException { + return execute(this, requireNonNull(handler), requireNonNull(catcher)); } @Override @@ -210,37 +210,38 @@ public abstract class AbstractConfigServerClient implements ConfigServerClient { /** Returns the mapped body, if successful, retrying any IOException. The caller must close the body stream. */ private <T> T mapIfSuccess(Function<InputStream, T> mapper) { - return handle((response, ioException) -> { - if (response != null) { - try { - InputStream body = response.getEntity() != null ? response.getEntity().getContent() - : InputStream.nullInputStream(); - if (response.getCode() >= HttpStatus.SC_REDIRECTION) - throw readException(body.readAllBytes()); - - return mapper.apply(new ForwardingInputStream(body) { - @Override - public void close() throws IOException { - super.close(); - response.close(); + return handle(response -> { + try { + InputStream body = response.getEntity() != null ? response.getEntity().getContent() + : InputStream.nullInputStream(); + if (response.getCode() >= HttpStatus.SC_REDIRECTION) + throw readException(body.readAllBytes()); + + return mapper.apply(new ForwardingInputStream(body) { + @Override + public void close() throws IOException { + super.close(); + response.close(); + } + }); } - }); - } - catch (IOException | RuntimeException | Error e) { - try { - response.close(); - } - catch (IOException f) { - e.addSuppressed(f); - } - if (e instanceof IOException) - ioException = (IOException) e; - else - sneakyThrow(e); - } - } - throw new RetryException(ioException); - }); + catch (IOException | RuntimeException | Error e) { + try { + response.close(); + } + catch (IOException f) { + e.addSuppressed(f); + } + if (e instanceof IOException) + throw new RetryException((IOException) e); + else + sneakyThrow(e); + throw new AssertionError("Should not happen"); + } + }, + ioException -> { + throw new RetryException(ioException); + }); } } diff --git a/configserver-client/src/main/java/ai/vespa/hosted/client/ConfigServerClient.java b/configserver-client/src/main/java/ai/vespa/hosted/client/ConfigServerClient.java index 234dbe9ee06..61b7edea0cc 100644 --- a/configserver-client/src/main/java/ai/vespa/hosted/client/ConfigServerClient.java +++ b/configserver-client/src/main/java/ai/vespa/hosted/client/ConfigServerClient.java @@ -12,6 +12,7 @@ import java.io.UncheckedIOException; import java.net.URI; import java.time.Duration; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.List; import java.util.function.BiFunction; @@ -33,7 +34,10 @@ public interface ConfigServerClient extends AutoCloseable { interface RequestBuilder { /** Sets the request path. */ - RequestBuilder at(String... pathSegments); + default RequestBuilder at(String... pathSegments) { return at(List.of(pathSegments)); } + + /** Sets the request path. */ + RequestBuilder at(List<String> pathSegments); /** Sets the request body as UTF-8 application/json. */ RequestBuilder body(byte[] json); @@ -42,24 +46,29 @@ public interface ConfigServerClient extends AutoCloseable { RequestBuilder body(HttpEntity entity); /** Sets the parameter key/values for the request. Number of arguments must be even. */ - RequestBuilder parameters(String... pairs); + default RequestBuilder parameters(String... pairs) { + return parameters(Arrays.asList(pairs)); + } + + /** Sets the parameter key/values for the request. Number of arguments must be even. */ + RequestBuilder parameters(List<String> pairs); /** Overrides the default socket read timeout of the request. {@code Duration.ZERO} gives infinite timeout. */ RequestBuilder timeout(Duration timeout); - /** Overrides the default socket read timeout of the request. {@code null} allows infinite timeout. */ + /** Overrides the default request config of the request. */ RequestBuilder config(RequestConfig config); /** * Sets custom retry/failure logic for this. * <p> - * Exactly one of the arguments (response, exception) are non-null. + * Exactly one of the callbacks will be invoked, with a non-null argument. * Return a value to have that returned to the caller; * throw a {@link RetryException} to have the request retried; or * throw any other unchecked exception to have this propagate out to the caller. * The caller must close the provided response, if any. */ - <T> T handle(BiFunction<ClassicHttpResponse, IOException, T> handler) throws UncheckedIOException; + <T> T handle(Function<ClassicHttpResponse, T> handler, Function<IOException, T> catcher) throws UncheckedIOException; /** Sets the response body mapper for this, for successful requests. */ <T> T read(Function<byte[], T> mapper) throws UncheckedIOException, ConfigServerException; diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/configserver/ConfigServer.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/configserver/ConfigServer.java index ed545dc35d1..81c4d7be483 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/configserver/ConfigServer.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/configserver/ConfigServer.java @@ -39,7 +39,7 @@ public interface ConfigServer { void reindex(DeploymentId deployment, List<String> clusterNames, List<String> documentTypes, boolean indexedOnly); - Optional<ApplicationReindexing> getReindexing(DeploymentId deployment); + ApplicationReindexing getReindexing(DeploymentId deployment); void disableReindexing(DeploymentId deployment); @@ -47,7 +47,7 @@ public interface ConfigServer { void restart(DeploymentId deployment, RestartFilter restartFilter); - void deactivate(DeploymentId deployment) throws NotFoundException; + void deactivate(DeploymentId deployment); boolean isSuspended(DeploymentId deployment); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java index 69d723edbe8..c85d23d58d1 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java @@ -183,8 +183,7 @@ public class ApplicationController { /** Returns the reindexing status for the given application in the given zone. */ public ApplicationReindexing applicationReindexing(ApplicationId id, ZoneId zoneId) { - return configServer.getReindexing(new DeploymentId(id, zoneId)) - .orElseThrow(() -> new NotExistsException("Reindexing status not found for " + id + " in " + zoneId)); + return configServer.getReindexing(new DeploymentId(id, zoneId)); } /** Enables reindexing for the given application in the given zone. */ @@ -678,8 +677,6 @@ public class ApplicationController { DeploymentId id = new DeploymentId(application.get().id().instance(instanceName), zone); try { configServer.deactivate(id); - } catch (NotFoundException ignored) { - // ok; already gone } finally { controller.routing().policies().refresh(application.get().id().instance(instanceName), application.get().deploymentSpec(), zone); if (zone.environment().isManuallyDeployed()) diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java index 65d3f666309..3dc88d5d6d2 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java @@ -532,8 +532,6 @@ public class JobController { var zone = type.zone(controller.system()); try { controller.serviceRegistry().configServer().deactivate(new DeploymentId(id.id(), zone)); - } catch (NotFoundException ignored) { - // Already gone -- great! } finally { // Passing an empty DeploymentSpec here is fine as it's used for registering global endpoint names, and // tester instances have none. diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java index ffb5e040517..78dab2cceb6 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java @@ -203,11 +203,11 @@ public class ApplicationApiHandler extends LoggingRequestHandler { catch (ConfigServerException e) { switch (e.getErrorCode()) { case NOT_FOUND: - return new ErrorResponse(NOT_FOUND, e.getErrorCode().name(), Exceptions.toMessageString(e)); + return ErrorResponse.notFoundError(Exceptions.toMessageString(e)); case ACTIVATION_CONFLICT: return new ErrorResponse(CONFLICT, e.getErrorCode().name(), Exceptions.toMessageString(e)); case INTERNAL_SERVER_ERROR: - return new ErrorResponse(INTERNAL_SERVER_ERROR, e.getErrorCode().name(), Exceptions.toMessageString(e)); + return ErrorResponse.internalServerError(Exceptions.toMessageString(e)); default: return new ErrorResponse(BAD_REQUEST, e.getErrorCode().name(), Exceptions.toMessageString(e)); } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ConfigServerMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ConfigServerMock.java index 70651ada473..ca52e31d04c 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ConfigServerMock.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ConfigServerMock.java @@ -432,21 +432,18 @@ public class ConfigServerMock extends AbstractComponent implements ConfigServer public void reindex(DeploymentId deployment, List<String> clusterNames, List<String> documentTypes, boolean indexedOnly) { } @Override - public Optional<ApplicationReindexing> getReindexing(DeploymentId deployment) { - return Optional.of(new ApplicationReindexing(true, - Map.of("cluster", - new ApplicationReindexing.Cluster(Map.of("type", 100L), - Map.of("type", new Status(Instant.ofEpochMilli(345), - Instant.ofEpochMilli(456), - Instant.ofEpochMilli(567), - ApplicationReindexing.State.FAILED, - "(＃｀д´)ﾉ", - 0.1)))))); - - + public ApplicationReindexing getReindexing(DeploymentId deployment) { + return new ApplicationReindexing(true, + Map.of("cluster", + new ApplicationReindexing.Cluster(Map.of("type", 100L), + Map.of("type", new Status(Instant.ofEpochMilli(345), + Instant.ofEpochMilli(456), + Instant.ofEpochMilli(567), + ApplicationReindexing.State.FAILED, + "(＃｀д´)ﾉ", + 0.1))))); } - @Override public void disableReindexing(DeploymentId deployment) { } @@ -464,12 +461,13 @@ public class ConfigServerMock extends AbstractComponent implements ConfigServer } @Override - public void deactivate(DeploymentId deployment) throws NotFoundException { + public void deactivate(DeploymentId deployment) { ApplicationId applicationId = deployment.applicationId(); nodeRepository().removeNodes(deployment.zoneId(), nodeRepository().list(deployment.zoneId(), applicationId)); if ( ! applications.containsKey(deployment)) - throw new NotFoundException("No application with id " + applicationId + " exists, cannot deactivate"); + return; + applications.remove(deployment); serviceStatus.remove(deployment); removeLoadBalancers(deployment.applicationId(), deployment.zoneId()); |