Merge pull request #4482 from vespa-engine/hakonhall/add-pause-between-retries-to-config-server

Add pause between retries to config server

2 files changed, 32 insertions, 37 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 3576f37eb9a..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,7 +43,6 @@ 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;
@@ -108,43 +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) {
-                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;
+        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;
+            }
+
+            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/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"));
     }