You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@curator.apache.org by "kezhuw (via GitHub)" <gi...@apache.org> on 2023/05/05 08:47:57 UTC
[GitHub] [curator] kezhuw opened a new pull request, #460: CURATOR-593: Append chroot for EnsembleProvider::setConnectionString in EnsembleTracker
kezhuw opened a new pull request, #460:
URL: https://github.com/apache/curator/pull/460
Curator uses `EnsembleProvider::getConnectionString` as connection string to `ZooKeeper`. `EnsembleTracker` subscribes to config node to construct up to date connection string for `EnsembleProvider`. This is great.
But, currently, `EnsembleTracker` omits chroot part of connection string which could cause curator locating at ZooKeeper root after reconnection. This could damage clients' data hierarchy in unexpected manner.
--
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
[GitHub] [curator] tisonkun commented on a diff in pull request #460: CURATOR-593: Append chroot for EnsembleProvider::setConnectionString in EnsembleTracker
Posted by "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
[GitHub] [curator] kezhuw commented on a diff in pull request #460: CURATOR-593: Append chroot for EnsembleProvider::setConnectionString in EnsembleTracker
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on code in PR #460:
URL: https://github.com/apache/curator/pull/460#discussion_r1200124392
##########
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:
Added `newClient(String connectionString, CountDownLatch ensembleLatch)`. The `withEnsembleProvider` parameter will be `true` if `ensembleLatch != null` in this overload.
--
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
[GitHub] [curator] tisonkun commented on a diff in pull request #460: CURATOR-593: Append chroot for EnsembleProvider::setConnectionString in EnsembleTracker
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #460:
URL: https://github.com/apache/curator/pull/460#discussion_r1201677225
##########
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();
Review Comment:
Is it possible that the `connectionString` also has a `chroot` part and we append the old chroot last which is disordered?
--
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
[GitHub] [curator] kezhuw commented on a diff in pull request #460: CURATOR-593: Append chroot for EnsembleProvider::setConnectionString in EnsembleTracker
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on code in PR #460:
URL: https://github.com/apache/curator/pull/460#discussion_r1200119273
##########
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:
Fixed in fixup commit.
--
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
[GitHub] [curator] kezhuw commented on a diff in pull request #460: CURATOR-593: Append chroot for EnsembleProvider::setConnectionString in EnsembleTracker
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on code in PR #460:
URL: https://github.com/apache/curator/pull/460#discussion_r1201688142
##########
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();
Review Comment:
No. This `connectionString` is constructed from `QuorumMaj` which is a server side concept and has no notion of `chroot`.
--
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
[GitHub] [curator] tisonkun commented on a diff in pull request #460: CURATOR-593: Append chroot for EnsembleProvider::setConnectionString in EnsembleTracker
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #460:
URL: https://github.com/apache/curator/pull/460#discussion_r1199691448
##########
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 when `i > 0` if it's an error state.
--
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
[GitHub] [curator] tisonkun commented on a diff in pull request #460: CURATOR-593: Append chroot for EnsembleProvider::setConnectionString in EnsembleTracker
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #460:
URL: https://github.com/apache/curator/pull/460#discussion_r1201674219
##########
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:
I get it.
--
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
[GitHub] [curator] kezhuw commented on pull request #460: CURATOR-593: Append chroot for EnsembleProvider::setConnectionString in EnsembleTracker
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on PR #460:
URL: https://github.com/apache/curator/pull/460#issuecomment-1558721022
Thank you for your reviewing. @tisonkun
--
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
[GitHub] [curator] kezhuw merged pull request #460: CURATOR-593: Append chroot for EnsembleProvider::setConnectionString in EnsembleTracker
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw merged PR #460:
URL: https://github.com/apache/curator/pull/460
--
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
[GitHub] [curator] kezhuw commented on a diff in pull request #460: CURATOR-593: Append chroot for EnsembleProvider::setConnectionString in EnsembleTracker
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on code in PR #460:
URL: https://github.com/apache/curator/pull/460#discussion_r1200117766
##########
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:
Hmm, we are extracting `chroot` from connection string. Everything last "/" (including "/" itself") are `chroot`.
The `if` part is similar to zookeeper.
https://github.com/apache/zookeeper/blob/a64dbf5b06ca1a73dc2ad6c7d31e679e312082aa/zookeeper-server/src/main/java/org/apache/zookeeper/client/ConnectStringParser.java#L55-L67
--
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