Merge pull request #15218 from vespa-engine/revert-15207-bjorncs/rewrite-config-convergence-checker-client

Revert "Bjorncs/rewrite config convergence checker client"

10 files changed, 94 insertions, 115 deletions

diff --git a/configserver/pom.xml b/configserver/pom.xml
index 43187038f97..8cd1b4b4254 100644
--- a/configserver/pom.xml
+++ b/configserver/pom.xml
@@ -220,11 +220,6 @@
       <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 3160c3eeca2..6b316c06b54 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,28 +1,27 @@
 // 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.VespaHttpClientBuilder;
+import ai.vespa.util.http.VespaClientBuilderFactory;
 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 com.yahoo.log.LogLevel;
+import java.util.logging.Level;
 import com.yahoo.slime.Cursor;
 import com.yahoo.vespa.config.server.http.JSONResponse;
-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 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 java.net.URI;
-import java.net.URISyntaxException;
 import java.time.Duration;
 import java.util.ArrayList;
 import java.util.LinkedHashMap;
@@ -30,7 +29,6 @@ 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;
 
@@ -44,11 +42,12 @@ 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,
@@ -59,12 +58,16 @@ public class ConfigConvergenceChecker extends AbstractComponent {
             "distributor"
     );
 
-    private final CloseableHttpClient httpClient;
-    private final ObjectMapper jsonMapper = new ObjectMapper();
+    private final StateApiFactory stateApiFactory;
+    private final VespaClientBuilderFactory clientBuilderFactory = new VespaClientBuilderFactory();
 
     @Inject
     public ConfigConvergenceChecker() {
-        this.httpClient = createHttpClient();
+        this(ConfigConvergenceChecker::createStateApi);
+    }
+
+    public ConfigConvergenceChecker(StateApiFactory stateApiFactory) {
+        this.stateApiFactory = stateApiFactory;
     }
 
     /** Fetches the active config generation for all services in the given application. */
@@ -79,7 +82,7 @@ public class ConfigConvergenceChecker extends AbstractComponent {
     }
 
     /** Check all services in given application. Returns the minimum current generation of all services */
-    public JSONResponse getServiceConfigGenerationsResponse(Application application, URI requestUrl, Duration timeoutPerService) {
+    public ServiceListResponse getServiceConfigGenerationsResponse(Application application, URI requestUrl, Duration timeoutPerService) {
         Map<ServiceInfo, Long> 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 +90,7 @@ public class ConfigConvergenceChecker extends AbstractComponent {
     }
 
     /** Check service identified by host and port in given application */
-    public JSONResponse getServiceConfigGenerationResponse(Application application, String hostAndPortToCheck, URI requestUrl, Duration timeout) {
+    public ServiceResponse getServiceConfigGenerationResponse(Application application, String hostAndPortToCheck, URI requestUrl, Duration timeout) {
         Long wantedGeneration = application.getApplicationGeneration();
         try {
             if ( ! hostInApplication(application, hostAndPortToCheck))
@@ -96,7 +99,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 (NonSuccessStatusCodeException | IOException e) { // e.g. if we cannot connect to the service to find generation
+        } catch (ProcessingException 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());
@@ -105,47 +108,46 @@ public class ConfigConvergenceChecker extends AbstractComponent {
 
     @Override
     public void deconstruct() {
-        try {
-            httpClient.close();
-        } catch (IOException e) {
-            throw new UncheckedIOException(e);
-        }
+        clientBuilderFactory.close();
+    }
+
+    @Path(statePath)
+    public interface StateApi {
+        @Path(configSubPath)
+        @GET
+        JsonNode config();
+    }
+
+    public interface StateApiFactory {
+        StateApi createStateApi(Client client, URI serviceUri);
     }
 
     /** 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 (IOException | NonSuccessStatusCodeException e) {
-                                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 (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
+                                                ));
     }
 
     /** 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.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;
+    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();
         }
     }
 
@@ -164,6 +166,16 @@ 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"))
@@ -175,43 +187,9 @@ public class ConfigConvergenceChecker extends AbstractComponent {
         return state.get("config").get("generation").asLong(-1);
     }
 
-    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 StateApi createStateApi(Client client, URI uri) {
+        WebTarget target = client.target(uri);
+        return WebResourceFactory.newResource(StateApi.class, target);
     }
 
     private static class ServiceListResponse extends JSONResponse {
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 05d1119aa4f..cc452421d2d 100644
--- a/configserver/src/main/java/com/yahoo/vespa/serviceview/ConfigServerLocation.java
+++ b/configserver/src/main/java/com/yahoo/vespa/serviceview/ConfigServerLocation.java
@@ -15,7 +15,6 @@ 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 138e6c8798c..76e600d2ad8 100644
--- a/configserver/src/main/java/com/yahoo/vespa/serviceview/StateResource.java
+++ b/configserver/src/main/java/com/yahoo/vespa/serviceview/StateResource.java
@@ -40,7 +40,6 @@ 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/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 6ed4dbf0c66..6aeb774d2b0 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,14 +184,10 @@ 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 ->
-                        assertThat(responseBody)
-                                .startsWith("{\"url\":\"" + requestUrl.toString() + "\",\"host\":\"" + hostAndPort(requestUrl) +
-                                        "\",\"wantedGeneration\":3,\"error\":\"Read timed out")
-                                .endsWith("\"}"),
-                404,
-                response);
+        assertResponse((responseBody) -> assertTrue("Response matches", responseBody.startsWith(
+                "{\"url\":\"" + requestUrl.toString() + "\",\"host\":\"" + hostAndPort(requestUrl) +
+                "\",\"wantedGeneration\":3,\"error\":\"java.net.SocketTimeoutException") &&
+                                      responseBody.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 06a455954ac..7276091fed0 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,6 +1,7 @@
 // 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;
@@ -17,6 +18,7 @@ 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;
@@ -35,10 +37,12 @@ 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;
 
@@ -421,6 +425,21 @@ 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);
     }
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 c6afa889041..2bac7f66799 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,10 +27,8 @@ 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 6d1c1c71f21..bdc89d737d4 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,13 +17,10 @@ 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 e2e769f8556..4b82f278f23 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,7 +14,6 @@ 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 309d6a756f6..95fdd61563b 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,7 +27,6 @@ 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);