From 4c9229905d2544ab58b3d93632d684bb30ce9a4a Mon Sep 17 00:00:00 2001 From: Håkon Hallingstad Date: Tue, 19 Dec 2017 11:17:31 +0100 Subject: Rely on tick for retries against same config server for node admin --- .../util/ConfigServerHttpRequestExecutor.java | 75 +++++++++------------- .../noderepository/NodeRepositoryImplTest.java | 1 - .../util/ConfigServerHttpRequestExecutorTest.java | 4 +- 3 files changed, 30 insertions(+), 50 deletions(-) (limited to 'node-admin') 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 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 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 tryAllConfigServers(CreateRequest requestFactory, Class 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 retryableException = HttpException.handleStatusCode( + response.getStatusLine().getStatusCode(), + "Config server " + configServer); + if (retryableException.isPresent()) { + lastException = retryableException.get(); continue; } try { - Optional 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")); } -- cgit v1.2.3