Hi all, Thank you for your information.
First, I agree that we should not introduce backward-incompatible changes in patch releases. Second, I have opened pr-1996[1] to fix the `getConfig` watcher path to "/zookeeper/config". Third, apologize for mistakes I made in the initial email. A. It is ZOOKEEPER-4565[2] that fixes chroot "/short" from receiving an illegal path "eper/config", not ZOOKEEPER-4601[3] which is the target of pr-1996. B. > For chroot "/zookeeper", watcher will receive event path "/config" This is not correct. I concluded this without writing a test to verify this. This misleads the discussion. In pursuing pr for ZOOKEEPER-4601[3], I found that `getConfig `watcher will receive no event with chroot "/zookeeper" or "/zookeeper/config". The cause is dual: * `getConfig` registers watch using path "/zookeeper/config".[4] * Event path "/zookeeper/config" is stripped to "/config" or "/" for chroot "/zookeeper" or "/zookeeper/config" accordingly. This is true for code before[5] or after[6] ZOOKEEPER-4565. * Finally, the stripped path does not match registered path "/zookeeper/config" So, if we fix the `getConfig` watcher path to "/zookeeper/config", it breaks no old code. Hence, safe to backport to stable releases. Besides pr-1996, I have pushed testcase branches based on master[7] and 3.8.0[8](which does not contain ZOOKEEPER-4565) in my fork for verification. The testcase commit is cherry-pick clean for branch 3.7 and 3.8. Finally, pr-1996 fixes `getConfig` watcher path with chroot "/zoo", it causes disconnection currently due to illegal path "keeper/config". I ignored this in ZOOKEEPER-4565. [1]: https://github.com/apache/zookeeper/pull/1996 [2]: https://issues.apache.org/jira/browse/ZOOKEEPER-4565 [3]: https://issues.apache.org/jira/browse/ZOOKEEPER-4601 [4]: https://github.com/apache/zookeeper/blob/89c1831f84891f425f1fa9224210587124f1c1ec/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java#L2057-L2064 [5]: https://github.com/apache/zookeeper/blob/05b215994f5e145c2758c4089828b57ba471b329/zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java#L886-L897 [6]: https://github.com/apache/zookeeper/blob/89c1831f84891f425f1fa9224210587124f1c1ec/zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java#L858-L869 [7]: https://github.com/kezhuw/zookeeper/tree/ZOOKEEPER-4601-get-config-watcher-path-testcase [8]: https://github.com/kezhuw/zookeeper/tree/ZOOKEEPER-4601-get-config-watcher-path-testcase-3.8.0 Best, Kezhu Wang On Sun, Apr 16, 2023 at 9:52 PM Enrico Olivelli <eolive...@gmail.com> wrote: > Il Ven 14 Apr 2023, 20:47 Chris Nauroth <cnaur...@apache.org> ha scritto: > > > Thank you for the discussion, Kezhu. > > > > My take is that the behavior you proposed, with the received path always > > /zookeeper/config regardless of chroot, is the correct one. However, it's > > hard to know if anyone has coded workarounds on top of the current > > behavior, which would then break if we made the change. > > > > The safest course of action would be to target a fix for 3.9.0 and > announce > > it as a backward-incompatible change. I don't think anyone has a specific > > timeline yet for a 3.9.0 release though. > > > > I'm curious to hear if others disagree with me and instead think the risk > > is low enough to make the change in existing release lines. > > > > I agree with this proposal. > > > Enrico > > > > > Chris Nauroth > > > > > > On Mon, Apr 3, 2023 at 9:34 AM Kezhu Wang <kez...@gmail.com> wrote: > > > > > Hi all, > > > > > > Any thoughts on this? > > > > > > Curator has similar issue reported as CURATOR-666[1]. I think it might > be > > > worth to get Curator and ZooKeeper behave same in `getConfig`. > > > > > > I replied this also to dev@curator for joint discussion. See my quotes > > and > > > ZOOKEEPER-4565[2], ZOOKEEPER-4601[3] for context. > > > > > > [1]: https://issues.apache.org/jira/browse/CURATOR-666 > > > [2]: https://issues.apache.org/jira/browse/ZOOKEEPER-4565 > > > [3]: https://issues.apache.org/jira/browse/ZOOKEEPER-4601 > > > > > > On Wed, Jul 27, 2022 at 8:35 PM Kezhu Wang <kez...@gmail.com> wrote: > > > > > > > Hi Devs, > > > > > > > > Before ZOOKEEPER-4565[1], `ClientCnxn` uses following code to strip > > > chroot: > > > > > > > > ``` > > > > // convert from a server path to a client path > > > > if (chrootPath != null) { > > > > String serverPath = event.getPath(); > > > > if (serverPath.compareTo(chrootPath) == 0) { > > > > event.setPath("/"); > > > > } else if (serverPath.length() > chrootPath.length()) { > > > > event.setPath(serverPath.substring(chrootPath.length())); > > > > } else { > > > > LOG.warn("Got server path {} which is too short for chroot > path > > > > {}.", > > > > event.getPath(), chrootPath); > > > > } > > > > } > > > > ``` > > > > > > > > This results in behavior: > > > > * For chroot "/zookeeper", watcher will receive event path "/config". > > > > * For chroot "/short", watcher will receive illegal path > "eper/config". > > > > This causes in ZOOKEEPER-4601. > > > > * For chroot "/pretty-long-chroot-path", watcher will receive event > > path > > > > "/zookeeper/config". > > > > > > > > ZOOKEEPEER-4601 changed the stripping code to fix illegal path: > > > > > > > > ``` > > > > private String stripChroot(String serverPath) { > > > > if (serverPath.startsWith(chrootPath)) { > > > > if (serverPath.length() == chrootPath.length()) { > > > > return "/"; > > > > } > > > > return serverPath.substring(chrootPath.length()); > > > > } else if > (serverPath.startsWith(ZooDefs.ZOOKEEPER_NODE_SUBTREE)) { > > > > return serverPath; > > > > } > > > > LOG.warn("Got server path {} which is not descendant of chroot > path > > > > {}.", serverPath, chrootPath); > > > > return serverPath; > > > > } > > > > ``` > > > > > > > > This results in behavior: > > > > * For chroot "/zookeeper", watcher will receive event path "/config". > > > > * For chroot "/other-chroot"(eg. "/short", > "/pretty-long-chroot-path", > > > > etc.), watcher will receive event path "/zookeeper/config". > > > > > > > > The path `getConfig` watcher received was not changed in > > ZOOKEEPER-4565. > > > > > > > > It is a bit of surprising to me that event path of `getConfig` > watcher > > is > > > > not "/zookeeper/config" . I guess the current behavior might not be > by > > > > design. Personally, I prefer to `getConfig` watchers(whether it is > the > > > > default one or not) to receive path "/zookeeper/config". But, > > obviously, > > > > such change is a breaking change in behavior(though might not be by > > > design) > > > > of public API. @eolivelli mentioned that such a change needs a > mailing > > > list > > > > decision, so I post it here for discussion in addition to > > > ZOOKEEPER-4601[2]. > > > > > > > > Any thoughts ? > > > > > > > > [1]: https://issues.apache.org/jira/browse/ZOOKEEPER-4565 > > > > [2]: https://issues.apache.org/jira/browse/ZOOKEEPER-4601 > > > > > > > > > > > > Best, > > > > Kezhu Wang > > > > > > > > > > > > > -- > > > Best, > > > Kezhu Wang > > > > > >
