[GitHub] [curator] tisonkun commented on a diff in pull request #460: CURATOR-593: Append chroot for EnsembleProvider::setConnectionString in EnsembleTracker

["tisonkun (via GitHub)" <gi...@apache.org>]

tisonkun commented on code in PR #460:
URL: https://github.com/apache/curator/pull/460#discussion_r1199690493


##########
curator-framework/src/test/java/org/apache/curator/framework/imps/TestReconfiguration.java:
##########
@@ -436,6 +439,51 @@ public void testNewMembers() throws Exception
         }
     }
 
+    @Test
+    public void testRemoveWithChroot() throws Exception
+    {
+        // Use a long chroot path to circumvent ZOOKEEPER-4565 and ZOOKEEPER-4601
+        String chroot = "/pretty-long-chroot";
+
+        try (CuratorFramework client = newClient(cluster.getConnectString() + chroot)) {
+            client.start();
+            client.create().forPath("/", "deadbeef".getBytes());
+
+            QuorumVerifier oldConfig = toQuorumVerifier(client.getConfig().forEnsemble());
+            assertConfig(oldConfig, cluster.getInstances());
+
+            CountDownLatch latch = setChangeWaiter(client);
+
+            Collection<InstanceSpec> oldInstances = cluster.getInstances();
+            InstanceSpec us = cluster.findConnectionInstance(client.getZookeeperClient().getZooKeeper());
+            InstanceSpec removeSpec = oldInstances.iterator().next();
+            if ( us.equals(removeSpec) ) {
+                Iterator<InstanceSpec> iterator = oldInstances.iterator();
+                iterator.next();
+                removeSpec = iterator.next();
+            }
+
+            client.reconfig().leaving(Integer.toString(removeSpec.getServerId())).fromConfig(oldConfig.getVersion()).forEnsemble();
+
+            assertTrue(timing.awaitLatch(latch));
+
+            byte[] newConfigData = client.getConfig().forEnsemble();
+            QuorumVerifier newConfig = toQuorumVerifier(newConfigData);
+            List<InstanceSpec> newInstances = Lists.newArrayList(cluster.getInstances());
+            newInstances.remove(removeSpec);
+            assertConfig(newConfig, newInstances);
+
+            assertTrue(timing.awaitLatch(ensembleLatch));
+            String connectString = EnsembleTracker.configToConnectionString(newConfig) + chroot;
+            assertEquals(connectString, ensembleProvider.getConnectionString());
+
+            client.getZookeeperClient().reset();
+            client.sync().forPath("/");
+            byte[] data = client.getData().forPath("/");
+            assertArrayEquals("deadbeef".getBytes(), data, () -> "expected \"deedbeef\", got data: " + Arrays.toString(data));

Review Comment:
   ```suggestion
               assertThat(new String(data)).isEqualTo("deadbeef");
   ```
   
   Simplify the assertion



##########
curator-framework/src/main/java/org/apache/curator/framework/imps/EnsembleTracker.java:
##########
@@ -209,10 +209,17 @@ private void processConfigData(byte[] data) throws Exception
         if (!properties.isEmpty())
         {
             QuorumMaj newConfig = new QuorumMaj(properties);
-            String connectionString = configToConnectionString(newConfig);
-            if (connectionString.trim().length() > 0)
+            String connectionString = configToConnectionString(newConfig).trim();
+            if (!connectionString.isEmpty())
             {
                 currentConfig.set(newConfig);
+                String oldConnectionString = ensembleProvider.getConnectionString();
+                int i = oldConnectionString.indexOf('/');
+                if (i >= 0)

Review Comment:
   Perhap `== 0`? I'm not sure what if `i > 0` the string constructing.



##########
curator-framework/src/test/java/org/apache/curator/framework/imps/TestReconfiguration.java:
##########
@@ -558,6 +606,7 @@ private CuratorFramework newClient(String connectionString) {
     private CuratorFramework newClient(String connectionString, boolean withEnsembleProvider)
     {
         final AtomicReference<String> connectString = new AtomicReference<>(connectionString);
+        ensembleLatch = new CountDownLatch(1);

Review Comment:
   I suggest to add a `newClient(String connectionString, boolean withEnsembleProvider, CountDownLatch)` overload to avoid using field - this is somehow global variable that is brittle.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@curator.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org