Merge pull request #2037 from yahoo/hmusum/use-random-config-server

Use random config server host

2 files changed, 33 insertions, 9 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 6af2816f653..e94b1f3bcd9 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
@@ -23,6 +23,9 @@ import java.io.BufferedReader;
 import java.io.IOException;
 import java.io.InputStreamReader;
 import java.io.UnsupportedEncodingException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
 import java.util.Optional;
 import java.util.Set;
 import java.util.stream.Collectors;
@@ -76,10 +79,14 @@ public class ConfigServerHttpRequestExecutor {
         }
     }
 
-    public <T> T tryAllConfigServers(CreateRequest requestFactory, Class<T> wantedReturnType) {
+    private <T> T tryAllConfigServers(CreateRequest requestFactory, Class<T> wantedReturnType) {
         Exception lastException = null;
+        // Shuffle config server hosts to balance load
+        List<String> shuffledConfigServerHosts = new ArrayList<>(configServerHosts);
+        Collections.shuffle(shuffledConfigServerHosts);
+
         for (int loopRetry = 0; loopRetry < MAX_LOOPS; loopRetry++) {
-            for (String configServer : configServerHosts) {
+            for (String configServer : shuffledConfigServerHosts) {
                 final CloseableHttpResponse response;
                 try {
                     response = client.execute(requestFactory.createRequest(configServer));
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 cd59a5d998a..2fcb3df9b82 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
@@ -17,10 +17,17 @@ import java.io.ByteArrayInputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.nio.charset.StandardCharsets;
+import java.util.Arrays;
+import java.util.List;
 import java.util.Set;
 
+import static org.hamcrest.CoreMatchers.endsWith;
 import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.CoreMatchers.startsWith;
+import static org.hamcrest.Matchers.arrayContainingInAnyOrder;
+import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.junit.MatcherAssert.assertThat;
+import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 import static org.mockito.Matchers.any;
 import static org.mockito.Mockito.doNothing;
@@ -40,8 +47,8 @@ public class ConfigServerHttpRequestExecutorTest {
         public String foo;
     }
 
-    final StringBuilder mockLog = new StringBuilder();
-    int mockReturnCode = 200;
+    private final StringBuilder mockLog = new StringBuilder();
+    private int mockReturnCode = 200;
 
     private CloseableHttpClient createClientMock() throws IOException {
         CloseableHttpClient httpMock = mock(CloseableHttpClient.class);
@@ -71,7 +78,7 @@ public class ConfigServerHttpRequestExecutorTest {
         ConfigServerHttpRequestExecutor executor = new ConfigServerHttpRequestExecutor(configServers, createClientMock());
         TestPojo answer = executor.get("/path", 666, TestPojo.class);
         assertThat(answer.foo, is("bar"));
-        assertThat(mockLog.toString(), is("GET http://host1:666/path  "));
+        assertLogStringContainsGETForAHost();
     }
 
     @Test
@@ -88,7 +95,7 @@ public class ConfigServerHttpRequestExecutorTest {
         } catch (Exception e) {
             // ignore
         }
-        assertThat(mockLog.toString(), is("GET http://host1:666/path  "));
+        assertLogStringContainsGETForAHost();
     }
 
     @Test
@@ -105,8 +112,10 @@ public class ConfigServerHttpRequestExecutorTest {
         } catch (Exception e) {
             // ignore
         }
-        assertThat(mockLog.toString(), is("GET http://host1:666/path  GET http://host2:666/path  " +
-                "GET http://host1:666/path  GET http://host2:666/path  "));
+
+        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
@@ -123,6 +132,14 @@ public class ConfigServerHttpRequestExecutorTest {
         } catch (ConfigServerHttpRequestExecutor.NotFoundException e) {
             // ignore
         }
-        assertThat(mockLog.toString(), is("GET http://host1:666/path  "));
+        assertLogStringContainsGETForAHost();
+    }
+
+    private void assertLogStringContainsGETForAHost() {
+        String logString = mockLog.toString();
+        //assertThat(logString, startsWith("GET http://host"));
+        //assertThat(logString, endsWith(":666/path  "));
+        assertTrue("log does not contain expected entries:" + logString,
+                   (logString.equals("GET http://host1:666/path  ") || logString.equals("GET http://host2:666/path  ")));
     }
 }
\ No newline at end of file