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. 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 >
