Retry on non-client HTTP errors

3 files changed, 51 insertions, 15 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 f7d6b86ce69..e825fa4ac8f 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
@@ -91,9 +91,13 @@ public class ConfigServerHttpRequestExecutor {
                 }
 
                 try {
-                    HttpException.throwOnFailure(
+                    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);
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/HttpException.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/HttpException.java
index f4e8c63235d..55d3ecc4e60 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/HttpException.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/HttpException.java
@@ -2,6 +2,7 @@
 package com.yahoo.vespa.hosted.node.admin.util;
 
 import javax.ws.rs.core.Response;
+import java.util.Optional;
 
 @SuppressWarnings("serial")
 public class HttpException extends RuntimeException {
@@ -12,25 +13,33 @@ public class HttpException extends RuntimeException {
         }
     }
 
-    static void throwOnFailure(int statusCode, String message) {
+    /**
+     * Returns empty on success.
+     * Returns an exception if the error is retriable.
+     * Throws an exception on a non-retriable error, like 404 Not Found.
+     */
+    static Optional<HttpException> handleStatusCode(int statusCode, String message) {
         Response.Status status = Response.Status.fromStatusCode(statusCode);
         if (status == null) {
-            throw new HttpException(statusCode, message);
+            return Optional.of(new HttpException(statusCode, message));
         }
 
-        if (status.getFamily() == Response.Status.Family.SUCCESSFUL) {
-            return;
+        switch (status.getFamily()) {
+            case SUCCESSFUL: return Optional.empty();
+            case CLIENT_ERROR:
+                switch (status) {
+                    case NOT_FOUND:
+                        throw new NotFoundException(message);
+                    case CONFLICT:
+                        // A response body is assumed to be present, and
+                        // will later be interpreted as an error.
+                        return Optional.empty();
+                }
+                throw new HttpException(statusCode, message);
         }
 
-        switch (status) {
-            case NOT_FOUND: throw new NotFoundException(message);
-            case CONFLICT:
-                // A response body is assumed to be present, and
-                // will later be interpreted as an error.
-                return;
-            default:
-                throw new HttpException(status, message);
-        }
+        // Other errors like server-side errors are assumed to be retryable.
+        return Optional.of(new HttpException(status, message));
     }
 
     private HttpException(int statusCode, String message) {
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 21d53593b65..d431f175b1c 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
@@ -113,7 +113,8 @@ public class ConfigServerHttpRequestExecutorTest {
         configServers.add("host2");
         // Client is throwing exception, should be retries.
         mockReturnCode = 100000;
-        ConfigServerHttpRequestExecutor executor = new ConfigServerHttpRequestExecutor(configServers, createClientMock());
+        ConfigServerHttpRequestExecutor executor =
+                new ConfigServerHttpRequestExecutor(configServers, createClientMock());
         try {
             executor.get("/path", 666, TestPojo.class);
             fail("Expected failure");
@@ -127,6 +128,28 @@ public class ConfigServerHttpRequestExecutorTest {
     }
 
     @Test
+    public void testRetriesOnBadHttpResponseCode() throws Exception {
+        Set<String> configServers = new ArraySet<>(2);
+        configServers.add("host1");
+        configServers.add("host2");
+        // Client is throwing exception, should be retries.
+        mockReturnCode = 503;
+        ConfigServerHttpRequestExecutor executor =
+                new ConfigServerHttpRequestExecutor(configServers, createClientMock());
+        try {
+            executor.get("/path", 666, TestPojo.class);
+            fail("Expected failure");
+        } catch (Exception e) {
+            // ignore
+        }
+
+        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"));
+    }
+
+    @Test
     public void testNotFound() throws Exception {
         Set<String> configServers = new ArraySet<>(2);
         configServers.add("host1");