You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2022/06/29 14:15:46 UTC
[GitHub] [zookeeper] kezhuw opened a new pull request, #1899: ZOOKEEPER-4565: Refine chroot strip to accommodate /zookeeper/ subtree
kezhuw opened a new pull request, #1899:
URL: https://github.com/apache/zookeeper/pull/1899
```java
if (serverPath.length() > chrootPath.length()) {
event.setPath(serverPath.substring(chrootPath.length()));
}
```
Currently, chroot strip code listed above could result in illegal path
(aka. path not start with "/"). This will disconnect zookeeper client
due to `StringIndexOutOfBoundsException` from `PathParentIterator.next`
in event handling.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on pull request #1899: ZOOKEEPER-4565: Refine chroot strip to accommodate /zookeeper/ subtree
Posted by GitBox <gi...@apache.org>.
symat commented on PR #1899:
URL: https://github.com/apache/zookeeper/pull/1899#issuecomment-1199420221
yes, I think it make sense. I'll try and cherry-pick this to the active branches.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on pull request #1899: ZOOKEEPER-4565: Refine chroot strip to accommodate /zookeeper/ subtree
Posted by GitBox <gi...@apache.org>.
symat commented on PR #1899:
URL: https://github.com/apache/zookeeper/pull/1899#issuecomment-1195118748
I'm merging this to master, thanks for the contribution @kezhuw and for the review @eolivelli !
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] kezhuw commented on a diff in pull request #1899: ZOOKEEPER-4565: Refine chroot strip to accommodate /zookeeper/ subtree
Posted by GitBox <gi...@apache.org>.
kezhuw commented on code in PR #1899:
URL: https://github.com/apache/zookeeper/pull/1899#discussion_r924503457
##########
zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java:
##########
@@ -855,6 +855,18 @@ class SendThread extends ZooKeeperThread {
private boolean isFirstConnect = true;
private volatile ZooKeeperSaslClient zooKeeperSaslClient;
+ private String stripChroot(String serverPath) {
Review Comment:
It uses `ClientCnxn.chrootPath` which is a non-static member.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on a diff in pull request #1899: ZOOKEEPER-4565: Refine chroot strip to accommodate /zookeeper/ subtree
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #1899:
URL: https://github.com/apache/zookeeper/pull/1899#discussion_r924426897
##########
zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java:
##########
@@ -855,6 +855,18 @@ class SendThread extends ZooKeeperThread {
private boolean isFirstConnect = true;
private volatile ZooKeeperSaslClient zooKeeperSaslClient;
+ private String stripChroot(String serverPath) {
Review Comment:
nit: static ?
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on pull request #1899: ZOOKEEPER-4565: Refine chroot strip to accommodate /zookeeper/ subtree
Posted by GitBox <gi...@apache.org>.
symat commented on PR #1899:
URL: https://github.com/apache/zookeeper/pull/1899#issuecomment-1199439278
I pushed to branch-3.8 and branch-3.7.
On branch-3.6 the cherry-pick failed as we have some conflict there. I'm not sure how important this fix is, but if you think its worth the time, then please submit a new PR against branch-3.6.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] kezhuw commented on pull request #1899: ZOOKEEPER-4565: Refine chroot strip to accommodate /zookeeper/ subtree
Posted by GitBox <gi...@apache.org>.
kezhuw commented on PR #1899:
URL: https://github.com/apache/zookeeper/pull/1899#issuecomment-1188893150
I found this in reproducing [CURATOR-593](https://issues.apache.org/jira/browse/CURATOR-593) and [CURATOR-611](https://issues.apache.org/jira/browse/CURATOR-611). Curator [EnsembleTracker](https://github.com/apache/curator/blob/master/curator-framework/src/main/java/org/apache/curator/framework/imps/EnsembleTracker.java) uses [getConfig](https://github.com/apache/curator/blob/master/curator-framework/src/main/java/org/apache/curator/framework/imps/EnsembleTracker.java#L157) to track zookeeper ensemble changes. It [ignores](https://github.com/apache/curator/blob/master/curator-framework/src/main/java/org/apache/curator/framework/imps/EnsembleTracker.java#L113) illegal path event from buggy stripping. But after ZooKeeper [3.6.3](https://github.com/apache/zookeeper/blob/release-3.6.3/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java#L627), the illegal path causes exception inside ZooKeeper client and hence disconnection. CURATOR-593/CURATOR-611 are blocked by this as
`getConfig` watcher will not receive event from chroot client.
> This is a special case which can be handled if needed. Doing the larger refactor would definitely break backward compatibility.
I agree. I think we can intercept `getConfig` watcher to pass "/zookeeper/config" to original watcher. This is a breaking change to old behavior though probably not by design. I guess it might be an opportunity to define and document semantics of `getConfig` watcher.
Anyway, semantics of `getConfig` watcher could be a separate issue. And this pr should not break anything.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] kezhuw commented on pull request #1899: ZOOKEEPER-4565: Refine chroot strip to accommodate /zookeeper/ subtree
Posted by GitBox <gi...@apache.org>.
kezhuw commented on PR #1899:
URL: https://github.com/apache/zookeeper/pull/1899#issuecomment-1170773182
One shortcoming of this fix and current design(aka. `ClientCnxn.chroot`) is that it does not work well with chroot `/zookeeper`.
* If we strip `chroot` first, then watcher from `getConfig` will get `/config` while it should be `/zookeeper/config`.
* If we match `/zookeeper` first, then watcher from `getData` will get `/zookeeper/config` while it should be `/config`.
Basically, I think it is a design flaw in client side(aka. `ZooKeeper`, `ClientCnxn` and `Watcher`). It might be better to store and strip `chroot` in `Watcher` and let `ClientCnxn` pass through server path directly to `Watcher`. But this requires major refactor, I guess it might not be worth.
PS. I made similar mistake(in outcoming's perspective) in [zookeeper-client-rust](https://github.com/kezhuw/zookeeper-client-rust/pull/5/files). But the fix to that project should cover above situations as strip is done [inside `Watcher`](https://github.com/kezhuw/zookeeper-client-rust/blob/f89b5bcbc03bb2d78c6afb36c5beccc8416badd0/src/client/watcher.rs#L50).
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] asfgit closed pull request #1899: ZOOKEEPER-4565: Refine chroot strip to accommodate /zookeeper/ subtree
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1899: ZOOKEEPER-4565: Refine chroot strip to accommodate /zookeeper/ subtree
URL: https://github.com/apache/zookeeper/pull/1899
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] kezhuw commented on pull request #1899: ZOOKEEPER-4565: Refine chroot strip to accommodate /zookeeper/ subtree
Posted by GitBox <gi...@apache.org>.
kezhuw commented on PR #1899:
URL: https://github.com/apache/zookeeper/pull/1899#issuecomment-1195466388
I guess we should backport this to all maintaining branches (aka. 3.6, 3.7 and 3.8) otherwise clients will get unexpected disconnected due to `getConfig` in chroot `ZooKeeper`. @symat @eolivelli
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on pull request #1899: ZOOKEEPER-4565: Refine chroot strip to accommodate /zookeeper/ subtree
Posted by GitBox <gi...@apache.org>.
symat commented on PR #1899:
URL: https://github.com/apache/zookeeper/pull/1899#issuecomment-1188763956
Thanks @kezhuw , I like your fix!
I think we should make sure we don't break e.g. curator. We had some automated tests for that AFAIR... @eolivelli , what do you think?
> One shortcoming of this fix and current design(aka. ClientCnxn.chroot) is that it does not work well with chroot /zookeeper.
it is true... although I wouldn't consider this a very big issue. This is a special case which can be handled if needed. Doing the larger refactor would definitely break backward compatibility.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on pull request #1899: ZOOKEEPER-4565: Refine chroot strip to accommodate /zookeeper/ subtree
Posted by GitBox <gi...@apache.org>.
symat commented on PR #1899:
URL: https://github.com/apache/zookeeper/pull/1899#issuecomment-1188972846
> I found this in reproducing [CURATOR-593](https://issues.apache.org/jira/browse/CURATOR-593) and [CURATOR-611](https://issues.apache.org/jira/browse/CURATOR-611).
I see! thanks for the context!
> Anyway, semantics of getConfig watcher could be a separate issue. And this pr should not break anything.
agree... I'll try and find a second reviewer.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org