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.

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
@@ -220,6 +220,11 @@
       <scope>test</scope>
     </dependency>
     <dependency>
+      <groupId>org.assertj</groupId>
+      <artifactId>assertj-core</artifactId>
+      <scope>test</scope>
+    </dependency>
+    <dependency>
       <groupId>com.yahoo.vespa</groupId>
       <artifactId>http-utils</artifactId>
       <version>${project.version}</version>
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<String> 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<ServiceInfo, Long> getServiceGenerations(List<ServiceInfo> 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<Integer> 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);
     }