Always reconfigure (idempotent), and to only non-retired servers

2 files changed, 21 insertions, 25 deletions

diff --git a/zookeeper-server/zookeeper-server-common/src/main/java/com/yahoo/vespa/zookeeper/Reconfigurer.java b/zookeeper-server/zookeeper-server-common/src/main/java/com/yahoo/vespa/zookeeper/Reconfigurer.java
index 514d45c7e77..8b46c67e0f6 100644
--- a/zookeeper-server/zookeeper-server-common/src/main/java/com/yahoo/vespa/zookeeper/Reconfigurer.java
+++ b/zookeeper-server/zookeeper-server-common/src/main/java/com/yahoo/vespa/zookeeper/Reconfigurer.java
@@ -57,7 +57,7 @@ public class Reconfigurer extends AbstractComponent {
             zooKeeperRunner = startServer(newConfig, server);
         }
 
-        if (shouldReconfigure(newConfig)) {
+        if (newConfig.dynamicReconfiguration()) {
             reconfigure(newConfig);
         }
         return peer;
@@ -73,23 +73,14 @@ public class Reconfigurer extends AbstractComponent {
         }
     }
 
-    private boolean shouldReconfigure(ZookeeperServerConfig newConfig) {
-        if (!newConfig.dynamicReconfiguration()) return false;
-        if (activeConfig == null) return false;
-        return ! newConfig.equals(activeConfig());
-    }
-
     private ZooKeeperRunner startServer(ZookeeperServerConfig zookeeperServerConfig, VespaZooKeeperServer server) {
         ZooKeeperRunner runner = new ZooKeeperRunner(zookeeperServerConfig, server);
         activeConfig = zookeeperServerConfig;
         return runner;
     }
 
-    // TODO jonmv: unify server config writing here and in Configurator
     // TODO jonmv: read dynamic file, discard if old quorum impossible (config file + .dynamic.<id>)
     // TODO jonmv: if dynamic file, all unlisted servers are observers; otherwise joiners are observers
-    // TODO jonmv: use bulk mode, i.e., supply only new servers
-    // TODO jonmv: always reconfigure when dynamic config is used, and to only non-retired nodes
     // TODO jonmv: verify reconfig by issuing a dummy write
     // TODO jonmv: wrap Curator in Provider, for Curator shutdown
     // TODO jonmv: scale down to 1 server as well
@@ -153,6 +144,7 @@ public class Reconfigurer extends AbstractComponent {
 
     private static List<String> servers(ZookeeperServerConfig config) {
         return config.server().stream()
+                     .filter(server -> ! server.retired())
                      .map(server -> serverSpec(server, config.clientPort(), false))
                      .collect(toList());
     }
diff --git a/zookeeper-server/zookeeper-server-common/src/test/java/com/yahoo/vespa/zookeeper/ReconfigurerTest.java b/zookeeper-server/zookeeper-server-common/src/test/java/com/yahoo/vespa/zookeeper/ReconfigurerTest.java
index b868fd65de3..bafc81fdca1 100644
--- a/zookeeper-server/zookeeper-server-common/src/test/java/com/yahoo/vespa/zookeeper/ReconfigurerTest.java
+++ b/zookeeper-server/zookeeper-server-common/src/test/java/com/yahoo/vespa/zookeeper/ReconfigurerTest.java
@@ -53,18 +53,13 @@ public class ReconfigurerTest {
         assertEquals("node1:2181", reconfigurer.connectionSpec());
         assertEquals("server.0=node0:2182:2183;2181,server.1=node1:2182:2183;2181,server.2=node2:2182:2183;2181,server.3=node3:2182:2183;2181,server.4=node4:2182:2183;2181",
                      reconfigurer.servers());
-        assertEquals(1, reconfigurer.reconfigurations());
-        assertSame(nextConfig, reconfigurer.activeConfig());
-
-        // No reconfiguration happens with same config
-        reconfigurer.startOrReconfigure(nextConfig);
-        assertEquals(1, reconfigurer.reconfigurations());
+        assertEquals(2, reconfigurer.reconfigurations());
         assertSame(nextConfig, reconfigurer.activeConfig());
 
         // Cluster shrinks
         nextConfig = createConfig(3, true);
         reconfigurer.startOrReconfigure(nextConfig);
-        assertEquals(2, reconfigurer.reconfigurations());
+        assertEquals(3, reconfigurer.reconfigurations());
         assertEquals("node1:2181", reconfigurer.connectionSpec());
         assertEquals("server.0=node0:2182:2183;2181,server.1=node1:2182:2183;2181,server.2=node2:2182:2183;2181",
                      reconfigurer.servers());
@@ -73,7 +68,7 @@ public class ReconfigurerTest {
         // Cluster loses node1, but node3 joins. Indices are shuffled.
         nextConfig = createConfig(3, true, 1);
         reconfigurer.startOrReconfigure(nextConfig);
-        assertEquals(3, reconfigurer.reconfigurations());
+        assertEquals(4, reconfigurer.reconfigurations());
         assertEquals("server.0=node0:2182:2183;2181,server.1=node2:2182:2183;2181,server.2=node3:2182:2183;2181",
                      reconfigurer.servers());
         assertSame(nextConfig, reconfigurer.activeConfig());
@@ -91,7 +86,7 @@ public class ReconfigurerTest {
         assertEquals("node1:2181", reconfigurer.connectionSpec());
         assertEquals("server.0=node0:2182:2183;2181,server.1=node1:2182:2183;2181,server.2=node2:2182:2183;2181,server.3=node3:2182:2183;2181,server.4=node4:2182:2183;2181",
                      reconfigurer.servers());
-        assertEquals(1, reconfigurer.reconfigurations());
+        assertEquals(2, reconfigurer.reconfigurations());
         assertSame(nextConfig, reconfigurer.activeConfig());
     }
 
@@ -112,24 +107,27 @@ public class ReconfigurerTest {
        reconfigurer.shutdown();
     }
 
-    private ZookeeperServerConfig createConfig(int numberOfServers, boolean dynamicReconfiguration, int... skipIndices) {
-        Arrays.sort(skipIndices);
+    private ZookeeperServerConfig createConfig(int numberOfServers, boolean dynamicReconfiguration, int... retiredIndices) {
+        Arrays.sort(retiredIndices);
         ZookeeperServerConfig.Builder builder = new ZookeeperServerConfig.Builder();
         builder.zooKeeperConfigFile(cfgFile.getAbsolutePath());
         builder.myidFile(idFile.getAbsolutePath());
         for (int i = 0, index = 0; i < numberOfServers; i++, index++) {
-            while (Arrays.binarySearch(skipIndices, index) >= 0) index++;
-            builder.server(newServer(i, "node" + index));
+            boolean retired = Arrays.binarySearch(retiredIndices, index) >= 0;
+            if (retired) i--;
+            builder.server(newServer(i, "node" + index, retired));
         }
+
         builder.myid(0);
         builder.dynamicReconfiguration(dynamicReconfiguration);
         return builder.build();
     }
 
-    private ZookeeperServerConfig.Server.Builder newServer(int id, String hostName) {
+    private ZookeeperServerConfig.Server.Builder newServer(int id, String hostName, boolean retired) {
         ZookeeperServerConfig.Server.Builder builder = new ZookeeperServerConfig.Server.Builder();
         builder.id(id);
         builder.hostname(hostName);
+        builder.retired(retired);
         return builder;
     }
 
@@ -142,6 +140,7 @@ public class ReconfigurerTest {
     private static class TestableReconfigurer extends Reconfigurer implements VespaZooKeeperServer {
 
         private final TestableVespaZooKeeperAdmin zooKeeperAdmin;
+        private final Phaser phaser = new Phaser(2);
         private QuorumPeer serverPeer;
 
         TestableReconfigurer(TestableVespaZooKeeperAdmin zooKeeperAdmin) {
@@ -157,6 +156,7 @@ public class ReconfigurerTest {
 
         void startOrReconfigure(ZookeeperServerConfig newConfig) {
             serverPeer = startOrReconfigure(newConfig, this, MockQuorumPeer::new);
+            phaser.arriveAndDeregister();
         }
 
         String connectionSpec() {
@@ -173,10 +173,14 @@ public class ReconfigurerTest {
 
         @Override
         public void shutdown() {
+            phaser.arriveAndAwaitAdvance();
             serverPeer.shutdown(Duration.ofSeconds(1)); }
 
         @Override
-        public void start(Path configFilePath) { serverPeer.start(configFilePath); }
+        public void start(Path configFilePath) {
+            phaser.arriveAndAwaitAdvance();
+            serverPeer.start(configFilePath);
+        }
 
         @Override
         public boolean reconfigurable() {