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