[GitHub] [zookeeper] kezhuw commented on pull request #1899: ZOOKEEPER-4565: Refine chroot strip to accommodate /zookeeper/ subtree

[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