More testing of reconfiguration

2 files changed, 70 insertions, 18 deletions

diff --git a/zookeeper-server/zookeeper-server-3.5.6/src/main/java/com/yahoo/vespa/zookeeper/Reconfigurer.java b/zookeeper-server/zookeeper-server-3.5.6/src/main/java/com/yahoo/vespa/zookeeper/Reconfigurer.java
index 99e981f5fc9..b92d49e2595 100644
--- a/zookeeper-server/zookeeper-server-3.5.6/src/main/java/com/yahoo/vespa/zookeeper/Reconfigurer.java
+++ b/zookeeper-server/zookeeper-server-3.5.6/src/main/java/com/yahoo/vespa/zookeeper/Reconfigurer.java
@@ -38,11 +38,11 @@ public class Reconfigurer extends AbstractComponent {
             zooKeeperRunner = startServer(newConfig);
 
         if (shouldReconfigure(newConfig))
-            reconfigure(newConfig);
+            reconfigure(new ReconfigurationInfo(existingConfig(), newConfig));
     }
 
     boolean shouldReconfigure(ZookeeperServerConfig newConfig) {
-        ZookeeperServerConfig existingConfig = zooKeeperRunner.zookeeperServerConfig();
+        ZookeeperServerConfig existingConfig = existingConfig();
         if (!newConfig.dynamicReconfiguration() || existingConfig == null) return false;
 
         return !newConfig.equals(existingConfig);
@@ -52,21 +52,16 @@ public class Reconfigurer extends AbstractComponent {
         return new ZooKeeperRunner(zookeeperServerConfig);
     }
 
-    void reconfigure(ZookeeperServerConfig newConfig) {
-        ZookeeperServerConfig existingConfig = zooKeeperRunner.zookeeperServerConfig();
-
-        List<String> originalServers = List.copyOf(servers(existingConfig));
-        log.log(Level.INFO, "Original servers: " + originalServers);
-
-        List<String> joiningServers = servers(newConfig);
-        List<String> leavingServers = setDifference(originalServers, joiningServers);
-        List<String> addedServers = setDifference(joiningServers, leavingServers);
+    void reconfigure(ReconfigurationInfo reconfigurationInfo) {
+        List<String> joiningServers = reconfigurationInfo.joiningServers();
+        List<String> leavingServers = reconfigurationInfo.leavingServers();
+        List<String> addedServers = reconfigurationInfo.addedServers();
 
         log.log(Level.INFO, "Will reconfigure zookeeper cluster. Joining servers: " + joiningServers +
                             ", leaving servers: " + leavingServers +
                             ", new members" + addedServers);
         try {
-            ZooKeeperAdmin zooKeeperAdmin = new ZooKeeperAdmin(connectionSpec(existingConfig), sessionTimeoutInSeconds, null);
+            ZooKeeperAdmin zooKeeperAdmin = new ZooKeeperAdmin(connectionSpec(reconfigurationInfo.existingConfig()), sessionTimeoutInSeconds, null);
 
             long fromConfig = -1;
             zooKeeperAdmin.reconfigure(joiningServers, leavingServers, addedServers, fromConfig, null);
@@ -78,7 +73,7 @@ public class Reconfigurer extends AbstractComponent {
     /**
      * Returns items in set a that are not in set b
      */
-    List<String> setDifference(List<String> a, List<String> b) {
+    static List<String> setDifference(List<String> a, List<String> b) {
         Set<String> ret = new HashSet<>(a);
         ret.removeAll(b);
         return new ArrayList<>(ret);
@@ -88,10 +83,48 @@ public class Reconfigurer extends AbstractComponent {
         return String.join(",", servers(config));
     }
 
-    private List<String> servers(ZookeeperServerConfig config) {
+    private static List<String> servers(ZookeeperServerConfig config) {
         return config.server().stream()
                 .map(server -> server.hostname() + ":" + server.quorumPort() + ":" + server.electionPort())
                 .collect(Collectors.toList());
     }
 
+    ZookeeperServerConfig existingConfig() {
+        return zooKeeperRunner.zookeeperServerConfig();
+    }
+
+    static class ReconfigurationInfo {
+
+        private final ZookeeperServerConfig existingConfig;
+        private final List<String> joiningServers;
+        private final List<String> leavingServers;
+        private final List<String> addedServers;
+
+        public ReconfigurationInfo(ZookeeperServerConfig existingConfig, ZookeeperServerConfig newConfig) {
+            this.existingConfig = existingConfig;
+            List<String> originalServers = List.copyOf(servers(existingConfig));
+
+            this.joiningServers = servers(newConfig);
+            this.leavingServers = setDifference(originalServers, servers(newConfig));
+            this.addedServers = setDifference(servers(newConfig), originalServers);
+        }
+
+        public ZookeeperServerConfig existingConfig() {
+            return existingConfig;
+        }
+
+        public List<String> joiningServers() {
+            return joiningServers;
+        }
+
+        public List<String> leavingServers() {
+            return leavingServers;
+        }
+
+        public List<String> addedServers() {
+            return addedServers;
+        }
+
+    }
+
 }
diff --git a/zookeeper-server/zookeeper-server-3.5.6/src/test/java/com/yahoo/vespa/zookeeper/ReconfigurerTest.java b/zookeeper-server/zookeeper-server-3.5.6/src/test/java/com/yahoo/vespa/zookeeper/ReconfigurerTest.java
index c04e55c83e0..41b7a065495 100644
--- a/zookeeper-server/zookeeper-server-3.5.6/src/test/java/com/yahoo/vespa/zookeeper/ReconfigurerTest.java
+++ b/zookeeper-server/zookeeper-server-3.5.6/src/test/java/com/yahoo/vespa/zookeeper/ReconfigurerTest.java
@@ -11,6 +11,7 @@ import java.io.File;
 import java.io.IOException;
 import java.util.stream.IntStream;
 
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
@@ -36,13 +37,17 @@ public class ReconfigurerTest {
     @Test
     public void testStartupAndReconfigure() {
         Reconfigurer reconfigurer = new Reconfigurer();
-        reconfigurer.startOrReconfigure(createConfig(1));
+        ZookeeperServerConfig existingConfig = createConfig(2);
+        reconfigurer.startOrReconfigure(existingConfig);
 
         // Created config has dynamicReconfig set to false
-        assertFalse(reconfigurer.shouldReconfigure(createConfig(2)));
+        assertFalse(reconfigurer.shouldReconfigure(createConfig(3)));
 
-        // Created config has dynamicReconfig set to true
-        assertTrue(reconfigurer.shouldReconfigure(createConfigAllowReconfiguring(2)));
+        // Increase number of servers, created config has dynamicReconfig set to true
+        assertReconfiguration(3, reconfigurer);
+
+        // Decrease number of servers, Created config has dynamicReconfig set to true
+        assertReconfiguration(1, reconfigurer);
 
         // Test that equal config does not cause reconfiguration
         Reconfigurer reconfigurer2 = new Reconfigurer();
@@ -79,4 +84,18 @@ public class ReconfigurerTest {
         return builder;
     }
 
+    private void assertReconfiguration(int numberOfServers, Reconfigurer reconfigurer) {
+        ZookeeperServerConfig existingConfig = reconfigurer.existingConfig();
+        int currentServerCount = reconfigurer.existingConfig().server().size();
+        int expectedLeavingServers = Math.max(0, currentServerCount - numberOfServers);
+        int expectedAddedServers = Math.max(0, numberOfServers - currentServerCount);
+
+        ZookeeperServerConfig newConfig = createConfigAllowReconfiguring(numberOfServers);
+        assertTrue(reconfigurer.shouldReconfigure(newConfig));
+        Reconfigurer.ReconfigurationInfo reconfigurationInfo = new Reconfigurer.ReconfigurationInfo(existingConfig, newConfig);
+        assertEquals(numberOfServers, reconfigurationInfo.joiningServers().size());
+        assertEquals(expectedLeavingServers, reconfigurationInfo.leavingServers().size());
+        assertEquals(expectedAddedServers, reconfigurationInfo.addedServers().size());
+    }
+
 }