You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by Kezhu Wang <ke...@gmail.com> on 2022/07/27 12:35:01 UTC
What path getConfig watchers should receive from ZooKeeper with chroot “/zookeeper/config” ?
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
Re: What path getConfig watchers should receive from ZooKeeper with chroot “/zookeeper/config” ?
Posted by Kezhu Wang <ke...@gmail.com>.
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 <eo...@gmail.com> wrote:
> Il Ven 14 Apr 2023, 20:47 Chris Nauroth <cn...@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 <ke...@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 <ke...@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
> > >
> >
>
Re: What path getConfig watchers should receive from ZooKeeper with chroot “/zookeeper/config” ?
Posted by Kezhu Wang <ke...@gmail.com>.
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 <eo...@gmail.com> wrote:
> Il Ven 14 Apr 2023, 20:47 Chris Nauroth <cn...@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 <ke...@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 <ke...@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
> > >
> >
>
Re: What path getConfig watchers should receive from ZooKeeper with chroot “/zookeeper/config” ?
Posted by Enrico Olivelli <eo...@gmail.com>.
Il Ven 14 Apr 2023, 20:47 Chris Nauroth <cn...@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 <ke...@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 <ke...@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
> >
>
Re: What path getConfig watchers should receive from ZooKeeper with chroot “/zookeeper/config” ?
Posted by Enrico Olivelli <eo...@gmail.com>.
Il Ven 14 Apr 2023, 20:47 Chris Nauroth <cn...@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 <ke...@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 <ke...@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
> >
>
Re: What path getConfig watchers should receive from ZooKeeper with chroot “/zookeeper/config” ?
Posted by Chris Nauroth <cn...@apache.org>.
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 <ke...@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 <ke...@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
>
Re: What path getConfig watchers should receive from ZooKeeper with chroot “/zookeeper/config” ?
Posted by Chris Nauroth <cn...@apache.org>.
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 <ke...@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 <ke...@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
>
Re: What path getConfig watchers should receive from ZooKeeper with chroot “/zookeeper/config” ?
Posted by Kezhu Wang <ke...@gmail.com>.
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 <ke...@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
Re: What path getConfig watchers should receive from ZooKeeper with chroot “/zookeeper/config” ?
Posted by Kezhu Wang <ke...@gmail.com>.
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 <ke...@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