Simplify tryAllConfigServers

2 files changed, 40 insertions, 53 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApiImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApiImpl.java
index 12ba777f018..aea44e728ad 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApiImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApiImpl.java
@@ -110,41 +110,26 @@ public class ConfigServerApiImpl implements ConfigServerApi {
     private <T> T tryAllConfigServers(CreateRequest requestFactory, Class<T> wantedReturnType) {
         Exception lastException = null;
         for (URI configServer : configServers) {
-            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 (CloseableHttpResponse response = client.execute(requestFactory.createRequest(configServer))) {
+                HttpException.handleStatusCode(
+                        response.getStatusLine().getStatusCode(), "Config server " + configServer);
 
                 try {
                     return mapper.readValue(response.getEntity().getContent(), wantedReturnType);
                 } catch (IOException e) {
-                    throw new RuntimeException("Response didn't contain nodes element, failed parsing?", e);
+                    throw new RuntimeException("Failed parse response from config server", e);
                 }
-            } finally {
-                try {
-                    response.close();
-                } catch (IOException e) {
-                    NODE_ADMIN_LOGGER.warning("Ignoring exception from closing response", e);
+            } catch (HttpException e) {
+                if (!e.isRetryable()) throw e;
+                lastException = e;
+            } catch (Exception e) {
+                // Failure to communicate with a config server is not abnormal during upgrades
+                if (e.getMessage().contains("(Connection refused)")) {
+                    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;
             }
         }
 
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/HttpException.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/HttpException.java
index d0f436d16b6..256fe38ec68 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/HttpException.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/HttpException.java
@@ -2,7 +2,6 @@
 package com.yahoo.vespa.hosted.node.admin.configserver;
 
 import javax.ws.rs.core.Response;
-import java.util.Optional;
 
 /**
  * @author hakonhall
@@ -10,35 +9,34 @@ import java.util.Optional;
 @SuppressWarnings("serial")
 public class HttpException extends RuntimeException {
 
-    public static class NotFoundException extends HttpException {
-
-        public NotFoundException(String message) {
-            super(Response.Status.NOT_FOUND, message);
-        }
+    private final boolean isRetryable;
 
+    private HttpException(int statusCode, String message, boolean isRetryable) {
+        super("HTTP status code " + statusCode + ": " + message);
+        this.isRetryable = isRetryable;
     }
 
-    public static class ForbiddenException extends HttpException {
-
-        public ForbiddenException(String message) {
-            super(Response.Status.FORBIDDEN, message);
-        }
+    private HttpException(Response.Status status, String message, boolean isRetryable) {
+        super(status.toString() + " (" + status.getStatusCode() + "): " + message);
+        this.isRetryable = isRetryable;
+    }
 
+    public boolean isRetryable() {
+        return isRetryable;
     }
 
     /**
-     * Returns empty on success.
-     * Returns an exception if the error is retriable.
-     * Throws an exception on a non-retriable error, like 404 Not Found.
+     * Returns on success.
+     * @throws HttpException for all non-expected status codes.
      */
-    static Optional<HttpException> handleStatusCode(int statusCode, String message) {
+    static void handleStatusCode(int statusCode, String message) {
         Response.Status status = Response.Status.fromStatusCode(statusCode);
         if (status == null) {
-            return Optional.of(new HttpException(statusCode, message));
+            throw new HttpException(statusCode, message, true);
         }
 
         switch (status.getFamily()) {
-            case SUCCESSFUL: return Optional.empty();
+            case SUCCESSFUL: return;
             case CLIENT_ERROR:
                 switch (status) {
                     case FORBIDDEN:
@@ -48,20 +46,24 @@ public class HttpException extends RuntimeException {
                     case CONFLICT:
                         // A response body is assumed to be present, and
                         // will later be interpreted as an error.
-                        return Optional.empty();
+                        return;
                 }
-                throw new HttpException(statusCode, message);
+                throw new HttpException(status, message, false);
         }
 
         // Other errors like server-side errors are assumed to be retryable.
-        return Optional.of(new HttpException(status, message));
+        throw new HttpException(status, message, true);
     }
 
-    private HttpException(int statusCode, String message) {
-        super("HTTP status code " + statusCode + ": " + message);
+    public static class NotFoundException extends HttpException {
+        public NotFoundException(String message) {
+            super(Response.Status.NOT_FOUND, message, false);
+        }
     }
 
-    private HttpException(Response.Status status, String message) {
-        super(status.toString() + " (" + status.getStatusCode() + "): " + message);
+    public static class ForbiddenException extends HttpException {
+        public ForbiddenException(String message) {
+            super(Response.Status.FORBIDDEN, message, false);
+        }
     }
 }