From 6a419fc0d2bed5e49f89990ef4bc647cf50d1f08 Mon Sep 17 00:00:00 2001 From: Bjørn Christian Seime Date: Fri, 6 Nov 2020 11:28:42 +0100 Subject: Rewrite ConfigConvergenceChecker to use Apache instead of Jersey client The effective exception handling for some confiserver APIs may have changed as WebApplicationException leaked out with old Jersey-based implementation. Connections will now be reused for a short duration. --- configserver/pom.xml | 5 + .../application/ConfigConvergenceChecker.java | 156 ++++++++++++--------- .../application/ConfigConvergenceCheckerTest.java | 17 ++- .../server/http/v2/ApplicationHandlerTest.java | 19 --- 4 files changed, 105 insertions(+), 92 deletions(-) diff --git a/configserver/pom.xml b/configserver/pom.xml index 8cd1b4b4254..43187038f97 100644 --- a/configserver/pom.xml +++ b/configserver/pom.xml @@ -219,6 +219,11 @@ mockito-core test + + org.assertj + assertj-core + test + com.yahoo.vespa http-utils diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/application/ConfigConvergenceChecker.java b/configserver/src/main/java/com/yahoo/vespa/config/server/application/ConfigConvergenceChecker.java index 6b316c06b54..e89b9685a06 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/application/ConfigConvergenceChecker.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/application/ConfigConvergenceChecker.java @@ -1,27 +1,28 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.config.server.application; -import ai.vespa.util.http.VespaClientBuilderFactory; +import ai.vespa.util.http.VespaHttpClientBuilder; import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; import com.google.inject.Inject; import com.yahoo.component.AbstractComponent; import com.yahoo.config.model.api.HostInfo; import com.yahoo.config.model.api.PortInfo; import com.yahoo.config.model.api.ServiceInfo; -import java.util.logging.Level; +import com.yahoo.log.LogLevel; import com.yahoo.slime.Cursor; import com.yahoo.vespa.config.server.http.JSONResponse; -import org.glassfish.jersey.client.ClientProperties; -import org.glassfish.jersey.client.proxy.WebResourceFactory; - -import javax.ws.rs.GET; -import javax.ws.rs.Path; -import javax.ws.rs.ProcessingException; -import javax.ws.rs.client.Client; -import javax.ws.rs.client.ClientRequestFilter; -import javax.ws.rs.client.WebTarget; -import javax.ws.rs.core.HttpHeaders; +import org.apache.http.HttpStatus; +import org.apache.http.client.config.RequestConfig; +import org.apache.http.client.methods.CloseableHttpResponse; +import org.apache.http.client.methods.HttpGet; +import org.apache.http.client.utils.URIBuilder; +import org.apache.http.impl.client.CloseableHttpClient; + +import java.io.IOException; +import java.io.UncheckedIOException; import java.net.URI; +import java.net.URISyntaxException; import java.time.Duration; import java.util.ArrayList; import java.util.LinkedHashMap; @@ -29,6 +30,7 @@ import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; +import java.util.concurrent.TimeUnit; import java.util.logging.Logger; import java.util.stream.Collectors; @@ -42,12 +44,11 @@ import static com.yahoo.config.model.api.container.ContainerServiceType.QRSERVER * * @author Ulf Lilleengen * @author hmusum + * @author bjorncs */ public class ConfigConvergenceChecker extends AbstractComponent { private static final Logger log = Logger.getLogger(ConfigConvergenceChecker.class.getName()); - private static final String statePath = "/state/v1/"; - private static final String configSubPath = "config"; private final static Set serviceTypesToCheck = Set.of( CONTAINER.serviceName, QRSERVER.serviceName, @@ -58,16 +59,12 @@ public class ConfigConvergenceChecker extends AbstractComponent { "distributor" ); - private final StateApiFactory stateApiFactory; - private final VespaClientBuilderFactory clientBuilderFactory = new VespaClientBuilderFactory(); + private final CloseableHttpClient httpClient; + private final ObjectMapper jsonMapper = new ObjectMapper(); @Inject public ConfigConvergenceChecker() { - this(ConfigConvergenceChecker::createStateApi); - } - - public ConfigConvergenceChecker(StateApiFactory stateApiFactory) { - this.stateApiFactory = stateApiFactory; + this.httpClient = createHttpClient(); } /** Fetches the active config generation for all services in the given application. */ @@ -99,7 +96,7 @@ public class ConfigConvergenceChecker extends AbstractComponent { long currentGeneration = getServiceGeneration(URI.create("http://" + hostAndPortToCheck), timeout); boolean converged = currentGeneration >= wantedGeneration; return ServiceResponse.createOkResponse(requestUrl, hostAndPortToCheck, wantedGeneration, currentGeneration, converged); - } catch (ProcessingException e) { // e.g. if we cannot connect to the service to find generation + } catch (NonSuccessStatusCodeException | IOException e) { // e.g. if we cannot connect to the service to find generation return ServiceResponse.createNotFoundResponse(requestUrl, hostAndPortToCheck, wantedGeneration, e.getMessage()); } catch (Exception e) { return ServiceResponse.createErrorResponse(requestUrl, hostAndPortToCheck, wantedGeneration, e.getMessage()); @@ -108,46 +105,47 @@ public class ConfigConvergenceChecker extends AbstractComponent { @Override public void deconstruct() { - clientBuilderFactory.close(); - } - - @Path(statePath) - public interface StateApi { - @Path(configSubPath) - @GET - JsonNode config(); - } - - public interface StateApiFactory { - StateApi createStateApi(Client client, URI serviceUri); + try { + httpClient.close(); + } catch (IOException e) { + throw new UncheckedIOException(e); + } } /** Gets service generation for a list of services (in parallel). */ private Map getServiceGenerations(List services, Duration timeout) { return services.parallelStream() - .collect(Collectors.toMap(service -> service, - service -> { - try { - return getServiceGeneration(URI.create("http://" + service.getHostName() - + ":" + getStatePort(service).get()), timeout); - } - catch (ProcessingException e) { // Cannot connect to service to determine service generation - return -1L; - } - }, - (v1, v2) -> { throw new IllegalStateException("Duplicate keys for values '" + v1 + "' and '" + v2 + "'."); }, - LinkedHashMap::new - )); + .collect(Collectors.toMap( + service -> service, + service -> { + try { + return getServiceGeneration(URI.create("http://" + service.getHostName() + + ":" + getStatePort(service).get()), timeout); + } catch (IOException | NonSuccessStatusCodeException e) { + return -1L; + } + }, + (v1, v2) -> { throw new IllegalStateException("Duplicate keys for values '" + v1 + "' and '" + v2 + "'."); }, + LinkedHashMap::new + )); } /** Get service generation of service at given URL */ - private long getServiceGeneration(URI serviceUrl, Duration timeout) { - Client client = createClient(timeout); - try { - StateApi state = stateApiFactory.createStateApi(client, serviceUrl); - return generationFromContainerState(state.config()); - } finally { - client.close(); + private long getServiceGeneration(URI serviceUrl, Duration timeout) throws IOException, NonSuccessStatusCodeException { + HttpGet request = new HttpGet(createApiUri(serviceUrl)); + request.setConfig(createRequestConfig(timeout)); + try (CloseableHttpResponse response = httpClient.execute(request)) { + int statusCode = response.getStatusLine().getStatusCode(); + if (statusCode != HttpStatus.SC_OK) throw new NonSuccessStatusCodeException(statusCode); + if (response.getEntity() == null) throw new IOException("Response has no content"); + JsonNode jsonContent = jsonMapper.readTree(response.getEntity().getContent()); + return generationFromContainerState(jsonContent); + } catch (Exception e) { + log.log( + LogLevel.DEBUG, + e, + () -> String.format("Failed to retrieve service config generation for '%s': %s", serviceUrl, e.getMessage())); + throw e; } } @@ -166,16 +164,6 @@ public class ConfigConvergenceChecker extends AbstractComponent { return false; } - private Client createClient(Duration timeout) { - return clientBuilderFactory.newBuilder() - .register( - (ClientRequestFilter) ctx -> - ctx.getHeaders().put(HttpHeaders.USER_AGENT, List.of("config-convergence-checker"))) - .property(ClientProperties.CONNECT_TIMEOUT, (int) timeout.toMillis()) - .property(ClientProperties.READ_TIMEOUT, (int) timeout.toMillis()) - .build(); - } - private static Optional getStatePort(ServiceInfo service) { return service.getPorts().stream() .filter(port -> port.getTags().contains("state")) @@ -187,9 +175,43 @@ public class ConfigConvergenceChecker extends AbstractComponent { return state.get("config").get("generation").asLong(-1); } - private static StateApi createStateApi(Client client, URI uri) { - WebTarget target = client.target(uri); - return WebResourceFactory.newResource(StateApi.class, target); + private static URI createApiUri(URI serviceUrl) { + try { + return new URIBuilder(serviceUrl) + .setPath("/state/v1/config") + .build(); + } catch (URISyntaxException e) { + throw new RuntimeException(e); + } + } + + private static CloseableHttpClient createHttpClient() { + return VespaHttpClientBuilder + .create() + .setUserAgent("config-convergence-checker") + .setConnectionTimeToLive(20, TimeUnit.SECONDS) + .setMaxConnPerRoute(4) + .setMaxConnTotal(100) + .setDefaultRequestConfig(createRequestConfig(Duration.ofSeconds(10))) + .build(); + } + + private static RequestConfig createRequestConfig(Duration timeout) { + int timeoutMillis = (int)timeout.toMillis(); + return RequestConfig.custom() + .setConnectionRequestTimeout(timeoutMillis) + .setConnectTimeout(timeoutMillis) + .setSocketTimeout(timeoutMillis) + .build(); + } + + private static class NonSuccessStatusCodeException extends Exception { + final int statusCode; + + NonSuccessStatusCodeException(int statusCode) { + super("Expected status code 200, got " + statusCode); + this.statusCode = statusCode; + } } private static class ServiceListResponse extends JSONResponse { diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/application/ConfigConvergenceCheckerTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/application/ConfigConvergenceCheckerTest.java index 6aeb774d2b0..1630e965771 100644 --- a/configserver/src/test/java/com/yahoo/vespa/config/server/application/ConfigConvergenceCheckerTest.java +++ b/configserver/src/test/java/com/yahoo/vespa/config/server/application/ConfigConvergenceCheckerTest.java @@ -2,12 +2,12 @@ package com.yahoo.vespa.config.server.application; import com.github.tomakehurst.wiremock.junit.WireMockRule; +import com.yahoo.component.Version; import com.yahoo.config.model.api.Model; import com.yahoo.config.provision.ApplicationId; import com.yahoo.config.provision.ApplicationName; import com.yahoo.config.provision.InstanceName; import com.yahoo.config.provision.TenantName; -import com.yahoo.component.Version; import com.yahoo.container.jdisc.HttpResponse; import com.yahoo.slime.Slime; import com.yahoo.slime.SlimeUtils; @@ -31,8 +31,8 @@ import static com.github.tomakehurst.wiremock.client.WireMock.get; import static com.github.tomakehurst.wiremock.client.WireMock.okJson; import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo; import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.options; +import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; /** * @author Ulf Lilleengen @@ -184,10 +184,15 @@ public class ConfigConvergenceCheckerTest { .withBody("response too slow"))); HttpResponse response = checker.getServiceConfigGenerationResponse(application, hostAndPort(service), requestUrl, Duration.ofMillis(1)); // Message contained in a SocketTimeoutException may differ across platforms, so we do a partial match of the response here - assertResponse((responseBody) -> assertTrue("Response matches", responseBody.startsWith( - "{\"url\":\"" + requestUrl.toString() + "\",\"host\":\"" + hostAndPort(requestUrl) + - "\",\"wantedGeneration\":3,\"error\":\"java.net.SocketTimeoutException") && - responseBody.endsWith("\"}")), 404, response); + assertResponse( + responseBody -> + assertThat(responseBody) + .startsWith("{\"url\":\"" + requestUrl.toString() + "\",\"host\":\"" + hostAndPort(requestUrl) + + "\",\"wantedGeneration\":3,\"error\":\"") + .contains("timed out") + .endsWith("\"}"), + 404, + response); } private URI testServer() { diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/ApplicationHandlerTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/ApplicationHandlerTest.java index 7276091fed0..06a455954ac 100644 --- a/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/ApplicationHandlerTest.java +++ b/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/ApplicationHandlerTest.java @@ -1,7 +1,6 @@ // Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.config.server.http.v2; -import com.fasterxml.jackson.databind.ObjectMapper; import com.yahoo.cloud.config.ConfigserverConfig; import com.yahoo.component.Version; import com.yahoo.config.model.api.ModelFactory; @@ -18,7 +17,6 @@ import com.yahoo.vespa.config.server.MockLogRetriever; import com.yahoo.vespa.config.server.MockProvisioner; import com.yahoo.vespa.config.server.MockTesterClient; import com.yahoo.vespa.config.server.TestComponentRegistry; -import com.yahoo.vespa.config.server.application.ConfigConvergenceChecker; import com.yahoo.vespa.config.server.application.HttpProxy; import com.yahoo.vespa.config.server.application.OrchestratorMock; import com.yahoo.vespa.config.server.deploy.DeployTester; @@ -37,12 +35,10 @@ import org.junit.Rule; import org.junit.Test; import org.junit.rules.TemporaryFolder; -import javax.ws.rs.client.Client; import java.io.ByteArrayInputStream; import java.io.File; import java.io.IOException; import java.io.InputStream; -import java.net.URI; import java.nio.charset.StandardCharsets; import java.util.List; @@ -425,21 +421,6 @@ public class ApplicationHandlerTest { return createApplicationHandler().handle(HttpRequest.createTestRequest(restartUrl, GET)); } - private static class MockStateApiFactory implements ConfigConvergenceChecker.StateApiFactory { - boolean createdApi = false; - @Override - public ConfigConvergenceChecker.StateApi createStateApi(Client client, URI serviceUri) { - createdApi = true; - return () -> { - try { - return new ObjectMapper().readTree("{\"config\":{\"generation\":1}}"); - } catch (IOException e) { - throw new RuntimeException(e); - } - }; - } - } - private ApplicationHandler createApplicationHandler() { return createApplicationHandler(applicationRepository); } -- cgit v1.2.3 From 024f41506371589e9be92e7c457ed965d0a4856a Mon Sep 17 00:00:00 2001 From: Bjørn Christian Seime Date: Fri, 6 Nov 2020 11:33:51 +0100 Subject: Don't use private inner class in return type of public methods --- .../vespa/config/server/application/ConfigConvergenceChecker.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/application/ConfigConvergenceChecker.java b/configserver/src/main/java/com/yahoo/vespa/config/server/application/ConfigConvergenceChecker.java index e89b9685a06..3160c3eeca2 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/application/ConfigConvergenceChecker.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/application/ConfigConvergenceChecker.java @@ -79,7 +79,7 @@ public class ConfigConvergenceChecker extends AbstractComponent { } /** Check all services in given application. Returns the minimum current generation of all services */ - public ServiceListResponse getServiceConfigGenerationsResponse(Application application, URI requestUrl, Duration timeoutPerService) { + public JSONResponse getServiceConfigGenerationsResponse(Application application, URI requestUrl, Duration timeoutPerService) { Map currentGenerations = getServiceConfigGenerations(application, timeoutPerService); long currentGeneration = currentGenerations.values().stream().mapToLong(Long::longValue).min().orElse(-1); return new ServiceListResponse(200, currentGenerations, requestUrl, application.getApplicationGeneration(), @@ -87,7 +87,7 @@ public class ConfigConvergenceChecker extends AbstractComponent { } /** Check service identified by host and port in given application */ - public ServiceResponse getServiceConfigGenerationResponse(Application application, String hostAndPortToCheck, URI requestUrl, Duration timeout) { + public JSONResponse getServiceConfigGenerationResponse(Application application, String hostAndPortToCheck, URI requestUrl, Duration timeout) { Long wantedGeneration = application.getApplicationGeneration(); try { if ( ! hostInApplication(application, hostAndPortToCheck)) -- cgit v1.2.3 From 6cb441ca82f721138b0518a10f6a7a6dfb91a970 Mon Sep 17 00:00:00 2001 From: Bjørn Christian Seime Date: Fri, 6 Nov 2020 11:44:28 +0100 Subject: Deprecate VespaClientBuilderFactory + VespaJerseyJaxRsClientFactory --- .../main/java/com/yahoo/vespa/serviceview/ConfigServerLocation.java | 1 + .../src/main/java/com/yahoo/vespa/serviceview/StateResource.java | 1 + .../src/main/java/ai/vespa/util/http/VespaClientBuilderFactory.java | 2 ++ .../com/yahoo/vespa/jaxrs/client/VespaJerseyJaxRsClientFactory.java | 3 +++ .../controller/RetryingClusterControllerClientFactory.java | 1 + .../controller/RetryingClusterControllerClientFactoryTest.java | 1 + 6 files changed, 9 insertions(+) diff --git a/configserver/src/main/java/com/yahoo/vespa/serviceview/ConfigServerLocation.java b/configserver/src/main/java/com/yahoo/vespa/serviceview/ConfigServerLocation.java index cc452421d2d..05d1119aa4f 100644 --- a/configserver/src/main/java/com/yahoo/vespa/serviceview/ConfigServerLocation.java +++ b/configserver/src/main/java/com/yahoo/vespa/serviceview/ConfigServerLocation.java @@ -15,6 +15,7 @@ public class ConfigServerLocation extends AbstractComponent { final int restApiPort; // The client factory must be owned by a component as StateResource is instantiated per request + @SuppressWarnings("removal") final VespaClientBuilderFactory clientBuilderFactory = new VespaClientBuilderFactory(); @Inject diff --git a/configserver/src/main/java/com/yahoo/vespa/serviceview/StateResource.java b/configserver/src/main/java/com/yahoo/vespa/serviceview/StateResource.java index 76e600d2ad8..138e6c8798c 100644 --- a/configserver/src/main/java/com/yahoo/vespa/serviceview/StateResource.java +++ b/configserver/src/main/java/com/yahoo/vespa/serviceview/StateResource.java @@ -40,6 +40,7 @@ public class StateResource implements StateClient { private static final String USER_AGENT = "service-view-config-server-client"; private static final String SINGLE_API_LINK = "url"; + @SuppressWarnings("removal") private final VespaClientBuilderFactory clientBuilderFactory; private final int restApiPort; private final String host; diff --git a/jaxrs_client_utils/src/main/java/ai/vespa/util/http/VespaClientBuilderFactory.java b/jaxrs_client_utils/src/main/java/ai/vespa/util/http/VespaClientBuilderFactory.java index 2bac7f66799..c6afa889041 100644 --- a/jaxrs_client_utils/src/main/java/ai/vespa/util/http/VespaClientBuilderFactory.java +++ b/jaxrs_client_utils/src/main/java/ai/vespa/util/http/VespaClientBuilderFactory.java @@ -27,8 +27,10 @@ import static java.util.logging.Level.CONFIG; * - hostname verification is not enabled - CN/SAN verification is assumed to be handled by the underlying x509 trust manager. * - ssl context or hostname verifier must not be overridden by the caller * + * @deprecated Use Apache httpclient based client factory instead (VespaHttpClientBuilder). * @author bjorncs */ +@Deprecated(forRemoval = true) public class VespaClientBuilderFactory implements AutoCloseable { private static final Logger log = Logger.getLogger(VespaClientBuilderFactory.class.getName()); diff --git a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/VespaJerseyJaxRsClientFactory.java b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/VespaJerseyJaxRsClientFactory.java index bdc89d737d4..6d1c1c71f21 100644 --- a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/VespaJerseyJaxRsClientFactory.java +++ b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/VespaJerseyJaxRsClientFactory.java @@ -17,10 +17,13 @@ import java.util.List; /** * Factory for creating Jersey based Vespa clients from a JAX-RS resource interface. * + * @deprecated Use Apache httpclient based client factory instead (VespaHttpClientBuilder). * @author bjorncs */ +@Deprecated(forRemoval = true) public class VespaJerseyJaxRsClientFactory implements JaxRsClientFactory, AutoCloseable { + @SuppressWarnings("removal") private final VespaClientBuilderFactory clientBuilder = new VespaClientBuilderFactory(); // Client is a heavy-weight object with a finalizer so we create only one and re-use it private final Client client; diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/RetryingClusterControllerClientFactory.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/RetryingClusterControllerClientFactory.java index 4b82f278f23..e2e769f8556 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/RetryingClusterControllerClientFactory.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/RetryingClusterControllerClientFactory.java @@ -14,6 +14,7 @@ import java.util.List; /** * @author bakksjo */ +@SuppressWarnings("removal") // VespaJerseyJaxRsClientFactory public class RetryingClusterControllerClientFactory extends AbstractComponent implements ClusterControllerClientFactory { // TODO: Figure this port out dynamically. diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/RetryingClusterControllerClientFactoryTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/RetryingClusterControllerClientFactoryTest.java index 95fdd61563b..309d6a756f6 100644 --- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/RetryingClusterControllerClientFactoryTest.java +++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/RetryingClusterControllerClientFactoryTest.java @@ -27,6 +27,7 @@ public class RetryingClusterControllerClientFactoryTest { private final Clock clock = new ManualClock(); @Test + @SuppressWarnings("removal") // VespaJerseyJaxRsClientFactory public void verifyJerseyCallForSetNodeState() throws IOException { VespaJerseyJaxRsClientFactory clientFactory = mock(VespaJerseyJaxRsClientFactory.class); ClusterControllerJaxRsApi api = mock(ClusterControllerJaxRsApi.class); -- cgit v1.2.3 From 4a9579145a299e14640462d351ad201594ab6abb Mon Sep 17 00:00:00 2001 From: Bjørn Christian Seime Date: Mon, 9 Nov 2020 12:01:55 +0100 Subject: Don't use shared fork join pool --- .../application/ConfigConvergenceChecker.java | 59 +++++++++++++++++----- 1 file changed, 47 insertions(+), 12 deletions(-) diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/application/ConfigConvergenceChecker.java b/configserver/src/main/java/com/yahoo/vespa/config/server/application/ConfigConvergenceChecker.java index 3160c3eeca2..1d753627bf9 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/application/ConfigConvergenceChecker.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/application/ConfigConvergenceChecker.java @@ -6,6 +6,7 @@ import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; import com.google.inject.Inject; import com.yahoo.component.AbstractComponent; +import com.yahoo.concurrent.DaemonThreadFactory; import com.yahoo.config.model.api.HostInfo; import com.yahoo.config.model.api.PortInfo; import com.yahoo.config.model.api.ServiceInfo; @@ -25,19 +26,24 @@ import java.net.URI; import java.net.URISyntaxException; import java.time.Duration; import java.util.ArrayList; -import java.util.LinkedHashMap; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; import java.util.logging.Logger; -import java.util.stream.Collectors; import static com.yahoo.config.model.api.container.ContainerServiceType.CLUSTERCONTROLLER_CONTAINER; import static com.yahoo.config.model.api.container.ContainerServiceType.CONTAINER; import static com.yahoo.config.model.api.container.ContainerServiceType.LOGSERVER_CONTAINER; import static com.yahoo.config.model.api.container.ContainerServiceType.QRSERVER; +import static java.util.stream.Collectors.toList; /** * Checks for convergence of config generation for a given application. @@ -61,6 +67,7 @@ public class ConfigConvergenceChecker extends AbstractComponent { private final CloseableHttpClient httpClient; private final ObjectMapper jsonMapper = new ObjectMapper(); + private final ExecutorService executor = createThreadpool(); @Inject public ConfigConvergenceChecker() { @@ -114,20 +121,33 @@ public class ConfigConvergenceChecker extends AbstractComponent { /** Gets service generation for a list of services (in parallel). */ private Map getServiceGenerations(List services, Duration timeout) { - return services.parallelStream() - .collect(Collectors.toMap( - service -> service, - service -> { + List> tasks = services.stream() + .map(service -> + (Callable) () -> { + long generation; try { - return getServiceGeneration(URI.create("http://" + service.getHostName() + generation = getServiceGeneration(URI.create("http://" + service.getHostName() + ":" + getStatePort(service).get()), timeout); } catch (IOException | NonSuccessStatusCodeException e) { - return -1L; + generation = -1L; } - }, - (v1, v2) -> { throw new IllegalStateException("Duplicate keys for values '" + v1 + "' and '" + v2 + "'."); }, - LinkedHashMap::new - )); + return new ServiceInfoWithGeneration(service, generation); + }) + .collect(toList()); + try { + List> taskResults = executor.invokeAll(tasks); + Map result = new HashMap<>(); + for (Future taskResult : taskResults) { + ServiceInfoWithGeneration info = taskResult.get(); + result.put(info.service, info.generation); + } + return result; + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new RuntimeException("Failed to retrieve config generation: " + e.getMessage(), e); + } catch (ExecutionException e) { + throw new RuntimeException("Failed to retrieve config generation: " + e.getMessage(), e); + } } /** Get service generation of service at given URL */ @@ -185,6 +205,11 @@ public class ConfigConvergenceChecker extends AbstractComponent { } } + private static ExecutorService createThreadpool() { + return Executors.newFixedThreadPool( + Runtime.getRuntime().availableProcessors(), new DaemonThreadFactory("config-convergence-checker-")); + } + private static CloseableHttpClient createHttpClient() { return VespaHttpClientBuilder .create() @@ -205,6 +230,16 @@ public class ConfigConvergenceChecker extends AbstractComponent { .build(); } + private static class ServiceInfoWithGeneration { + final ServiceInfo service; + final long generation; + + ServiceInfoWithGeneration(ServiceInfo service, long generation) { + this.service = service; + this.generation = generation; + } + } + private static class NonSuccessStatusCodeException extends Exception { final int statusCode; -- cgit v1.2.3 From bde08ee18a7a381fdf3b6a82b97d58e69db0f483 Mon Sep 17 00:00:00 2001 From: Bjørn Christian Seime Date: Mon, 9 Nov 2020 12:19:43 +0100 Subject: Don't reuse connections Disable connection reuse. Increase max simultaneous connections. Remove default request config. --- .../server/application/ConfigConvergenceChecker.java | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/application/ConfigConvergenceChecker.java b/configserver/src/main/java/com/yahoo/vespa/config/server/application/ConfigConvergenceChecker.java index 1d753627bf9..bdcc9783640 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/application/ConfigConvergenceChecker.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/application/ConfigConvergenceChecker.java @@ -36,7 +36,6 @@ import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.Future; -import java.util.concurrent.TimeUnit; import java.util.logging.Logger; import static com.yahoo.config.model.api.container.ContainerServiceType.CLUSTERCONTROLLER_CONTAINER; @@ -119,7 +118,10 @@ public class ConfigConvergenceChecker extends AbstractComponent { } } - /** Gets service generation for a list of services (in parallel). */ + /** + * Gets service generation for a list of services (in parallel). + * This should ideally be implemented using an async http client + * */ private Map getServiceGenerations(List services, Duration timeout) { List> tasks = services.stream() .map(service -> @@ -153,6 +155,7 @@ public class ConfigConvergenceChecker extends AbstractComponent { /** Get service generation of service at given URL */ private long getServiceGeneration(URI serviceUrl, Duration timeout) throws IOException, NonSuccessStatusCodeException { HttpGet request = new HttpGet(createApiUri(serviceUrl)); + request.addHeader("Connection", "close"); request.setConfig(createRequestConfig(timeout)); try (CloseableHttpResponse response = httpClient.execute(request)) { int statusCode = response.getStatusLine().getStatusCode(); @@ -214,10 +217,9 @@ public class ConfigConvergenceChecker extends AbstractComponent { return VespaHttpClientBuilder .create() .setUserAgent("config-convergence-checker") - .setConnectionTimeToLive(20, TimeUnit.SECONDS) - .setMaxConnPerRoute(4) - .setMaxConnTotal(100) - .setDefaultRequestConfig(createRequestConfig(Duration.ofSeconds(10))) + .setMaxConnPerRoute(10) + .setMaxConnTotal(400) + .setConnectionReuseStrategy((response, context) -> false) // Disable connection reuse .build(); } -- cgit v1.2.3 From 530b15ad64dc46818c86ffc9e7f0dfe4bf8fc0c1 Mon Sep 17 00:00:00 2001 From: Bjørn Christian Seime Date: Mon, 9 Nov 2020 16:10:10 +0100 Subject: Stabilize unit test Use LinkedHashMap for consistent iteration ordering. Use JsonTestHelper for proper semantic json comparison. --- .../server/application/ConfigConvergenceChecker.java | 4 ++-- .../server/application/ConfigConvergenceCheckerTest.java | 15 +++------------ 2 files changed, 5 insertions(+), 14 deletions(-) diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/application/ConfigConvergenceChecker.java b/configserver/src/main/java/com/yahoo/vespa/config/server/application/ConfigConvergenceChecker.java index bdcc9783640..7376452df42 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/application/ConfigConvergenceChecker.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/application/ConfigConvergenceChecker.java @@ -26,7 +26,7 @@ import java.net.URI; import java.net.URISyntaxException; import java.time.Duration; import java.util.ArrayList; -import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Optional; @@ -138,7 +138,7 @@ public class ConfigConvergenceChecker extends AbstractComponent { .collect(toList()); try { List> taskResults = executor.invokeAll(tasks); - Map result = new HashMap<>(); + Map result = new LinkedHashMap<>(); for (Future taskResult : taskResults) { ServiceInfoWithGeneration info = taskResult.get(); result.put(info.service, info.generation); diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/application/ConfigConvergenceCheckerTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/application/ConfigConvergenceCheckerTest.java index 1630e965771..4948432c646 100644 --- a/configserver/src/test/java/com/yahoo/vespa/config/server/application/ConfigConvergenceCheckerTest.java +++ b/configserver/src/test/java/com/yahoo/vespa/config/server/application/ConfigConvergenceCheckerTest.java @@ -9,8 +9,6 @@ import com.yahoo.config.provision.ApplicationName; import com.yahoo.config.provision.InstanceName; import com.yahoo.config.provision.TenantName; import com.yahoo.container.jdisc.HttpResponse; -import com.yahoo.slime.Slime; -import com.yahoo.slime.SlimeUtils; import com.yahoo.vespa.config.server.ServerCache; import com.yahoo.vespa.config.server.monitoring.MetricUpdater; import org.junit.Before; @@ -31,6 +29,7 @@ import static com.github.tomakehurst.wiremock.client.WireMock.get; import static com.github.tomakehurst.wiremock.client.WireMock.okJson; import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo; import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.options; +import static com.yahoo.test.json.JsonTestHelper.assertJsonEquals; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.assertEquals; @@ -207,16 +206,8 @@ public class ConfigConvergenceCheckerTest { return uri.getHost() + ":" + uri.getPort(); } - private static void assertResponse(String json, int status, HttpResponse response) { - assertResponse((responseBody) -> { - Slime expected = SlimeUtils.jsonToSlime(json.getBytes()); - Slime actual = SlimeUtils.jsonToSlime(responseBody.getBytes()); - try { - assertEquals(new String((SlimeUtils.toJsonBytes(expected))), new String(SlimeUtils.toJsonBytes(actual))); - } catch (IOException e) { - throw new UncheckedIOException(e); - } - }, status, response); + private static void assertResponse(String expectedJson, int status, HttpResponse response) { + assertResponse((responseBody) -> assertJsonEquals(new String(responseBody.getBytes()), expectedJson), status, response); } private static void assertResponse(Consumer assertFunc, int status, HttpResponse response) { -- cgit v1.2.3