Rely on tick for retries against same config server for node admin

3 files changed, 30 insertions, 50 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/ConfigServerHttpRequestExecutor.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/ConfigServerHttpRequestExecutor.java
index b1d49fba137..94ad94d9a65 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/ConfigServerHttpRequestExecutor.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/ConfigServerHttpRequestExecutor.java
@@ -43,12 +43,10 @@ import java.util.Optional;
  */
 public class ConfigServerHttpRequestExecutor {
     private static final PrefixLogger NODE_ADMIN_LOGGER = PrefixLogger.getNodeAdminLogger(ConfigServerHttpRequestExecutor.class);
-    private static final int MAX_LOOPS = 2;
 
     private final ObjectMapper mapper = new ObjectMapper();
     private final CloseableHttpClient client;
     private final List<URI> configServerHosts;
-    private int pauseBetweenRetriesMs = 10_000;
 
     @Override
     public void finalize() throws Throwable {
@@ -61,10 +59,6 @@ public class ConfigServerHttpRequestExecutor {
         super.finalize();
     }
 
-    public void eliminatePauseBetweenRetriesForTesting() {
-        pauseBetweenRetriesMs = 0;
-    }
-
     public static ConfigServerHttpRequestExecutor create(Collection<URI> configServerUris) {
         PoolingHttpClientConnectionManager cm = new PoolingHttpClientConnectionManager(getConnectionSocketFactoryRegistry());
         cm.setMaxTotal(200); // Increase max total connections to 200, which should be enough
@@ -113,52 +107,41 @@ public class ConfigServerHttpRequestExecutor {
 
     private <T> T tryAllConfigServers(CreateRequest requestFactory, Class<T> wantedReturnType) {
         Exception lastException = null;
-        for (int loopRetry = 0; loopRetry < MAX_LOOPS; loopRetry++) {
-            for (URI configServer : configServerHosts) {
-                if (lastException != null) {
-                    try {
-                        // Avoid overloading the config server
-                        Thread.sleep(pauseBetweenRetriesMs);
-                    } catch (InterruptedException e) {
-                        // Ignore
-                    }
+        for (URI configServer : configServerHosts) {
+            final CloseableHttpResponse response;
+            try {
+                response = client.execute(requestFactory.createRequest(configServer));
+            } catch (Exception e) {
+                // Failure to communicate with a config server is not abnormal, as they are
+                // upgraded at the same time as Docker hosts.
+                if (e.getMessage().indexOf("(Connection refused)") > 0) {
+                    NODE_ADMIN_LOGGER.info("Connection refused to " + configServer + " (upgrading?), will try next");
+                } else {
+                    NODE_ADMIN_LOGGER.warning("Failed to communicate with " + configServer + ", will try next: " + e.getMessage());
                 }
+                lastException = e;
+                continue;
+            }
 
-                final CloseableHttpResponse response;
-                try {
-                    response = client.execute(requestFactory.createRequest(configServer));
-                } catch (Exception e) {
-                    // Failure to communicate with a config server is not abnormal, as they are
-                    // upgraded at the same time as Docker hosts.
-                    if (e.getMessage().indexOf("(Connection refused)") > 0) {
-                        NODE_ADMIN_LOGGER.info("Connection refused to " + configServer + " (upgrading?), will try next");
-                    } else {
-                        NODE_ADMIN_LOGGER.warning("Failed to communicate with " + configServer + ", will try next: " + e.getMessage());
-                    }
-                    lastException = e;
+            try {
+                Optional<HttpException> retryableException = HttpException.handleStatusCode(
+                        response.getStatusLine().getStatusCode(),
+                        "Config server " + configServer);
+                if (retryableException.isPresent()) {
+                    lastException = retryableException.get();
                     continue;
                 }
 
                 try {
-                    Optional<HttpException> retryableException = HttpException.handleStatusCode(
-                            response.getStatusLine().getStatusCode(),
-                            "Config server " + configServer);
-                    if (retryableException.isPresent()) {
-                        lastException = retryableException.get();
-                        continue;
-                    }
-
-                    try {
-                        return mapper.readValue(response.getEntity().getContent(), wantedReturnType);
-                    } catch (IOException e) {
-                        throw new RuntimeException("Response didn't contain nodes element, failed parsing?", e);
-                    }
-                } finally {
-                    try {
-                        response.close();
-                    } catch (IOException e) {
-                        NODE_ADMIN_LOGGER.warning("Ignoring exception from closing response", e);
-                    }
+                    return mapper.readValue(response.getEntity().getContent(), wantedReturnType);
+                } catch (IOException e) {
+                    throw new RuntimeException("Response didn't contain nodes element, failed parsing?", e);
+                }
+            } finally {
+                try {
+                    response.close();
+                } catch (IOException e) {
+                    NODE_ADMIN_LOGGER.warning("Ignoring exception from closing response", e);
                 }
             }
         }
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/noderepository/NodeRepositoryImplTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/noderepository/NodeRepositoryImplTest.java
index dfc591d63d5..45f5de3d1a5 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/noderepository/NodeRepositoryImplTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/noderepository/NodeRepositoryImplTest.java
@@ -58,7 +58,6 @@ public class NodeRepositoryImplTest {
         final int port = findRandomOpenPort();
         requestExecutor = ConfigServerHttpRequestExecutor.create(
                 Collections.singleton(URI.create("http://127.0.0.1:" + port)));
-        requestExecutor.eliminatePauseBetweenRetriesForTesting();
         container = JDisc.fromServicesXml(ContainerConfig.servicesXmlV2(port), Networking.enable);
     }
 
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/util/ConfigServerHttpRequestExecutorTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/util/ConfigServerHttpRequestExecutorTest.java
index 67cd2c79034..799f8a72fd9 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/util/ConfigServerHttpRequestExecutorTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/util/ConfigServerHttpRequestExecutorTest.java
@@ -117,8 +117,7 @@ public class ConfigServerHttpRequestExecutorTest {
         }
 
         String[] log = mockLog.toString().split("  ");
-        assertThat(log, arrayContainingInAnyOrder("GET http://host1:666/path", "GET http://host2:666/path",
-                                         "GET http://host1:666/path", "GET http://host2:666/path"));
+        assertThat(log, arrayContainingInAnyOrder("GET http://host1:666/path", "GET http://host2:666/path"));
     }
 
     @Test
@@ -134,7 +133,6 @@ public class ConfigServerHttpRequestExecutorTest {
 
         String[] log = mockLog.toString().split("  ");
         assertThat(log, arrayContainingInAnyOrder(
-                "GET http://host1:666/path", "GET http://host2:666/path",
                 "GET http://host1:666/path", "GET http://host2:666/path"));
     }