You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2022/12/02 17:57:23 UTC
[GitHub] [zookeeper] PapaCharlie opened a new pull request, #1950: Communicate the Zxid that triggered a WatchEvent to fire
PapaCharlie opened a new pull request, #1950:
URL: https://github.com/apache/zookeeper/pull/1950
With the recent addition of persistent watches, many doors have opened up to significantly more performant and intuitive local caches of remote state, but the actual implementation can be difficult because to cache data locally, one needs to execute the following steps:
1. Set the watch
2. Bootstrap the watched subtree
3. Catch up on the events that fired during the bootstrap
The issue is it's now very difficult to deduplicate and sanely resolve the remote state during step 3 because it's unknown whether an event arrived during the bootstrap or after. For example, imagine that between steps 1 and 2, a node /a was deleted then re-created. By the time step 3 is executed, there will be a NodeDeleted event queued up followed by a NodeCreated, causing at best a double read (one from the bootstrap, one from the NodeCreated) or at worst some data inconsistencies in the local cache.
This change sets the Zxid in the response header whenever the watch event type is NodeCreated, NodeDeleted, NodeDataChanged or NodeChildrenChanged.
--
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
[GitHub] [zookeeper] PapaCharlie commented on pull request #1950: ZOOKEEPER-4655: Communicate the Zxid that triggered a WatchEvent to fire
Posted by GitBox <gi...@apache.org>.
PapaCharlie commented on PR #1950:
URL: https://github.com/apache/zookeeper/pull/1950#issuecomment-1376113488
@eolivelli Done
--
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
[GitHub] [zookeeper] gu0keno0 commented on pull request #1950: ZOOKEEPER-4655: Communicate the Zxid that triggered a WatchEvent to fire
Posted by "gu0keno0 (via GitHub)" <gi...@apache.org>.
gu0keno0 commented on PR #1950:
URL: https://github.com/apache/zookeeper/pull/1950#issuecomment-1557717414
>>> In persistent watch: There is https://github.com/apache/zookeeper/pull/1106#issuecomment-543860329. So all events during disconnection are simple [lost](https://github.com/kezhuw/zookeeper/commit/31d89e9829380559066fc2b83e3d38462380c5d4). Currently, Curator [rebuilt](https://github.com/apache/curator/blob/1e82d0c1d0708d84d06e5757e435d1d9dbebfb48/curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/CuratorCacheImpl.java#L90) on [reconnected](https://github.com/apache/curator/blob/1e82d0c1d0708d84d06e5757e435d1d9dbebfb48/curator-recipes/src/main/java/org/apache/curator/framework/recipes/watch/PersistentWatcher.java#L67).
My understanding of @PapaCharlie 's use case is that his cache would also reduild upon reconnection. And AFAICT, this feature should be able to simplify the rebuilding process, during which we re-read all Znodes from ZK and new persistent watch events may fire at the same time. Then as @PapaCharlie suggested in his initial statement, in this situation this new feature should be able to simplify the cache maintainenace logic, because if the persistent watch's txid is lower than / equal to the txid of the cached data, we know that the newer data is already read.
--
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
[GitHub] [zookeeper] PapaCharlie commented on pull request #1950: ZOOKEEPER-4655: Communicate the Zxid that triggered a WatchEvent to fire
Posted by "PapaCharlie (via GitHub)" <gi...@apache.org>.
PapaCharlie commented on PR #1950:
URL: https://github.com/apache/zookeeper/pull/1950#issuecomment-1459192780
Hey @kezhuw thanks so much for taking a look at this! Regarding your comment, yes without this feature building an eventually consistent cache can be tricky because it requires handling a lot of edge cases when a read does not match what's expected based on the watch events received. Depending on what the local cache does when it receives events, it could end up in a weird state. I banged my head against the wall enough trying to implement a nice consistent local cache that I opened this PR :)
Lucky for me there was room in the API!
--
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
[GitHub] [zookeeper] PapaCharlie commented on pull request #1950: ZOOKEEPER-4655: Communicate the Zxid that triggered a WatchEvent to fire
Posted by "PapaCharlie (via GitHub)" <gi...@apache.org>.
PapaCharlie commented on PR #1950:
URL: https://github.com/apache/zookeeper/pull/1950#issuecomment-1594091157
Hey @anmolnar and @eolivelli thanks for taking a look! Really appreciate the review. I didn't have an account so I asked @abhilash1in to update the ticket. I have an account now so I'll assign it to myself
--
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
[GitHub] [zookeeper] PapaCharlie commented on pull request #1950: ZOOKEEPER-4655: Communicate the Zxid that triggered a WatchEvent to fire
Posted by "PapaCharlie (via GitHub)" <gi...@apache.org>.
PapaCharlie commented on PR #1950:
URL: https://github.com/apache/zookeeper/pull/1950#issuecomment-1459093587
Finally was able to get all the checks to pass. Sorry about that! The branch is looking good now
--
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
[GitHub] [zookeeper] PapaCharlie commented on pull request #1950: ZOOKEEPER-4655: Communicate the Zxid that triggered a WatchEvent to fire
Posted by "PapaCharlie (via GitHub)" <gi...@apache.org>.
PapaCharlie commented on PR #1950:
URL: https://github.com/apache/zookeeper/pull/1950#issuecomment-1459522068
I think there's a flaky test with `WatcherCleanerTest.testDeadWatcherMetrics`. I can repro it locally it and it doesn't seem related to my change. I will try to fix it
--
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
[GitHub] [zookeeper] eolivelli commented on a diff in pull request #1950: ZOOKEEPER-4655: Communicate the Zxid that triggered a WatchEvent to fire
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on code in PR #1950:
URL: https://github.com/apache/zookeeper/pull/1950#discussion_r1201614118
##########
zookeeper-server/src/main/java/org/apache/zookeeper/WatchedEvent.java:
##########
@@ -31,27 +31,38 @@
*/
@InterfaceAudience.Public
public class WatchedEvent {
+ public static final long NO_ZXID = -1L;
private final KeeperState keeperState;
private final EventType eventType;
- private String path;
+ private final String path;
+ private final long zxid;
/**
- * Create a WatchedEvent with specified type, state and path
+ * Create a WatchedEvent with specified type, state, path and zxid
*/
- public WatchedEvent(EventType eventType, KeeperState keeperState, String path) {
+ public WatchedEvent(EventType eventType, KeeperState keeperState, String path, long zxid) {
Review Comment:
This is public API, we should keep the original constructors, maybe you can mark them "@Deprecated"
--
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
[GitHub] [zookeeper] kezhuw commented on pull request #1950: ZOOKEEPER-4655: Communicate the Zxid that triggered a WatchEvent to fire
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on PR #1950:
URL: https://github.com/apache/zookeeper/pull/1950#issuecomment-1557685392
> Regarding the multi-op API, will the order of watch event be different from the order of write operations in the same multi-op tx?
@gu0keno0 They are same from my knowledge.
> order the events in the main memory on the watch event consumer side, and deal with the watch events one by one to maintain the cache consistency
I think this is crucial. The "deal" should tolerate to disconnection. [`autoWatchReset`](https://issues.apache.org/jira/browse/ZOOKEEPER-43) is fragile on its own from my observation.
Says, "/foo" and "/bar" are watched in a sub tree. Two changes(`setData` for example) are made in server to "/foo" and "/bar" in order. The first "deal" succeed and update `lastZxid` in client side. The updated `lastZxid` covers change to "/bar". Assumes that the second "deal" fails due to connection issue.
* In standard watch: No event for "/bar" after reconnected as client's `lastZxid` is up to date. We have to retry "/bar" here.
* In persistent watch: There is [no `autoWatchReset`](https://github.com/apache/zookeeper/pull/1106#issuecomment-543860329). So all events during disconnection are simple [lost](https://github.com/kezhuw/zookeeper/commit/31d89e9829380559066fc2b83e3d38462380c5d4). Currently, Curator [rebuilt](https://github.com/apache/curator/blob/1e82d0c1d0708d84d06e5757e435d1d9dbebfb48/curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/CuratorCacheImpl.java#L90) on [reconnected](https://github.com/apache/curator/blob/1e82d0c1d0708d84d06e5757e435d1d9dbebfb48/curator-recipes/src/main/java/org/apache/curator/framework/recipes/watch/PersistentWatcher.java#L67).
> FWIW, we only need the cache to be eventually consistent, yet the simplification gain we can get from leveraging persistent watch will be a lot.
Though, persistent watch [does not work well](https://github.com/kezhuw/zookeeper/commit/31d89e9829380559066fc2b83e3d38462380c5d4#diff-cfd09b7021c88da6631872e8a4a271f830162f7c5a63a140839ba029048493fdR227-R230) with `autoWatchReset`, it is still a great feature.
--
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
[GitHub] [zookeeper] kezhuw commented on pull request #1950: ZOOKEEPER-4655: Communicate the Zxid that triggered a WatchEvent to fire
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on PR #1950:
URL: https://github.com/apache/zookeeper/pull/1950#issuecomment-1544455796
Just for info, [ZOOKEEPER-1289: Multi Op Watch Events](https://issues.apache.org/jira/browse/ZOOKEEPER-1289) is probably related.
--
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
[GitHub] [zookeeper] kezhuw commented on pull request #1950: ZOOKEEPER-4655: Communicate the Zxid that triggered a WatchEvent to fire
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on PR #1950:
URL: https://github.com/apache/zookeeper/pull/1950#issuecomment-1549940269
Hi @PapaCharlie, I have opened [ZOOKEEPER-4695](https://issues.apache.org/jira/browse/ZOOKEEPER-4695). I agree your point, I think it is a matter of breaking change.
> based on my experience in building a client-side cache, having the zxid in the header would resolve a lot of internal inconsistencies
Seems that you are building something new in another language, I think [ZooKeeper Watches](https://zookeeper.apache.org/doc/r3.8.1/zookeeperProgrammers.html#ch_zkWatches) could be help.
--
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
[GitHub] [zookeeper] PapaCharlie commented on pull request #1950: ZOOKEEPER-4655: Communicate the Zxid that triggered a WatchEvent to fire
Posted by "PapaCharlie (via GitHub)" <gi...@apache.org>.
PapaCharlie commented on PR #1950:
URL: https://github.com/apache/zookeeper/pull/1950#issuecomment-1479963379
@kezhuw can you take another look?
--
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
[GitHub] [zookeeper] anmolnar merged pull request #1950: ZOOKEEPER-4655: Communicate the Zxid that triggered a WatchEvent to fire
Posted by "anmolnar (via GitHub)" <gi...@apache.org>.
anmolnar merged PR #1950:
URL: https://github.com/apache/zookeeper/pull/1950
--
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
[GitHub] [zookeeper] gu0keno0 commented on pull request #1950: ZOOKEEPER-4655: Communicate the Zxid that triggered a WatchEvent to fire
Posted by "gu0keno0 (via GitHub)" <gi...@apache.org>.
gu0keno0 commented on PR #1950:
URL: https://github.com/apache/zookeeper/pull/1950#issuecomment-1553742525
@kezhuw : agreed that regular ZK watch can achieve the ordering the modification events; however, for maintaining an in-memory cache for a Znode tree, a single recursive persistent watch is easier to use than regular watches, which has to be set on each Znode in the tree to trigger events on Znode content changes.
Regarding the multi-op API, will the order of watch event be different from the order of write operations in the same multi-op tx? If the orders are the same (e.g. write op1, op2 ==> watch event op1 , watch event op2), then I'd say even if they have the same txid, we can still just order the events in the main memory on the watch event consumer side, and deal with the watch events one by one to maintain the cache consistency. FWIW, we only need the cache to be eventually consistent, yet the simplification gain we can get from leveraging persistent watch will be a lot.
--
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
[GitHub] [zookeeper] PapaCharlie commented on pull request #1950: ZOOKEEPER-4655: Communicate the Zxid that triggered a WatchEvent to fire
Posted by "PapaCharlie (via GitHub)" <gi...@apache.org>.
PapaCharlie commented on PR #1950:
URL: https://github.com/apache/zookeeper/pull/1950#issuecomment-1553575844
Yeah I am building something in go (https://github.com/go-zookeeper/zk/pull/89) but it still would help significantly to have the zxid in the watch.
I think, fundamentally, because this fits in the existing API, I don't think that the fact that the multis _can_ have this behavior is a strong enough reason _not_ to have this. This can greatly simplify client-side behavior in many applications and avoid double lookups in many circumstances.
--
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
[GitHub] [zookeeper] PapaCharlie commented on pull request #1950: ZOOKEEPER-4655: Communicate the Zxid that triggered a WatchEvent to fire
Posted by "PapaCharlie (via GitHub)" <gi...@apache.org>.
PapaCharlie commented on PR #1950:
URL: https://github.com/apache/zookeeper/pull/1950#issuecomment-1460594331
https://github.com/apache/zookeeper/pull/1989 @kezhuw I fixed the flaky test, can you take a look?
--
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
[GitHub] [zookeeper] PapaCharlie commented on pull request #1950: Communicate the Zxid that triggered a WatchEvent to fire
Posted by GitBox <gi...@apache.org>.
PapaCharlie commented on PR #1950:
URL: https://github.com/apache/zookeeper/pull/1950#issuecomment-1372345411
Hey @eolivelli, thanks for taking a look at this! Happy to hear this looks good. I'm just lucky there was room in the existing API for me to squeeze this.
Let me update the commit message and poke around the docs and see where I can make it more obvious that this is available.
Regarding the new client/old server issue: yeah old servers will always send -1, so it's a pretty clear way to know whether you're talking to an old server. I'll make sure to state that in the docs.
Let me take a look at curator and see what can be done. It may be feasible to expose this in a sane way
--
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
[GitHub] [zookeeper] PapaCharlie commented on pull request #1950: ZOOKEEPER-4655: Communicate the Zxid that triggered a WatchEvent to fire
Posted by "PapaCharlie (via GitHub)" <gi...@apache.org>.
PapaCharlie commented on PR #1950:
URL: https://github.com/apache/zookeeper/pull/1950#issuecomment-1456722953
Hey @eolivelli can you take another look at this? I'd really appreciate it!
--
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
[GitHub] [zookeeper] PapaCharlie commented on a diff in pull request #1950: ZOOKEEPER-4655: Communicate the Zxid that triggered a WatchEvent to fire
Posted by "PapaCharlie (via GitHub)" <gi...@apache.org>.
PapaCharlie commented on code in PR #1950:
URL: https://github.com/apache/zookeeper/pull/1950#discussion_r1128889826
##########
zookeeper-server/src/test/java/org/apache/zookeeper/server/watch/WatchManagerTest.java:
##########
@@ -430,28 +434,37 @@ public void testWatcherMetrics(String className) throws IOException {
final String path3 = "/path3";
- //both wather1 and wather2 are watching path1
+ //both watcher1 and watcher2 are watching path1
manager.addWatch(path1, watcher1);
manager.addWatch(path1, watcher2);
//path2 is watched by watcher1
manager.addWatch(path2, watcher1);
- manager.triggerWatch(path3, EventType.NodeCreated);
+ manager.triggerWatch(path3, EventType.NodeCreated, 1);
//path3 is not being watched so metric is 0
checkMetrics("node_created_watch_count", 0L, 0L, 0D, 0L, 0L);
+ // Watchers shouldn't have received any events yet so the zxid should be -1.
+ checkWatchedZxid(watcher1, -1);
+ checkWatchedZxid(watcher2, -1);
//path1 is watched by two watchers so two fired
- manager.triggerWatch(path1, EventType.NodeCreated);
+ manager.triggerWatch(path1, EventType.NodeCreated, 2);
checkMetrics("node_created_watch_count", 2L, 2L, 2D, 1L, 2L);
+ checkWatchedZxid(watcher1, 2);
+ checkWatchedZxid(watcher2, 2);
//path2 is watched by one watcher so one fired now total is 3
- manager.triggerWatch(path2, EventType.NodeCreated);
+ manager.triggerWatch(path2, EventType.NodeCreated, 3);
checkMetrics("node_created_watch_count", 1L, 2L, 1.5D, 2L, 3L);
+ checkWatchedZxid(watcher1, 3);
+ checkWatchedZxid(watcher2, 2);
//watches on path1 are no longer there so zero fired
- manager.triggerWatch(path1, EventType.NodeDataChanged);
+ manager.triggerWatch(path1, EventType.NodeDataChanged, 4);
checkMetrics("node_changed_watch_count", 0L, 0L, 0D, 0L, 0L);
+ checkWatchedZxid(watcher1, 3);
+ checkWatchedZxid(watcher2, 2);
//both wather1 and wather2 are watching path1
Review Comment:
Whoops, nice catch
--
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
[GitHub] [zookeeper] PapaCharlie commented on a diff in pull request #1950: ZOOKEEPER-4655: Communicate the Zxid that triggered a WatchEvent to fire
Posted by "PapaCharlie (via GitHub)" <gi...@apache.org>.
PapaCharlie commented on code in PR #1950:
URL: https://github.com/apache/zookeeper/pull/1950#discussion_r1128894889
##########
zookeeper-server/src/main/java/org/apache/zookeeper/WatchedEvent.java:
##########
@@ -66,9 +78,44 @@ public String getPath() {
return path;
}
+ /**
+ * Returns the zxid of the transaction that triggered this watch if it is
+ * of one of the following types:<ul>
+ * <li>{@link EventType#NodeCreated}</li>
+ * <li>{@link EventType#NodeDeleted}</li>
+ * <li>{@link EventType#NodeDataChanged}</li>
+ * <li>{@link EventType#NodeChildrenChanged}</li>
+ * </ul>
+ * Otherwise, returns {@value #NO_ZXID}. Note that {@value #NO_ZXID} is also
+ * returned by old servers that do not support this feature.
+ */
+ public long getZxid() {
+ return zxid;
+ }
+
@Override
public String toString() {
- return "WatchedEvent state:" + keeperState + " type:" + eventType + " path:" + path;
+ return "WatchedEvent state:" + keeperState + " type:" + eventType + " path:" + path + " zxid: " + zxid;
+ }
+
+ @Override
+ public boolean equals(Object o) {
Review Comment:
You're right, let me double check whether it's actually necessary to have this
--
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
[GitHub] [zookeeper] kezhuw commented on a diff in pull request #1950: ZOOKEEPER-4655: Communicate the Zxid that triggered a WatchEvent to fire
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on code in PR #1950:
URL: https://github.com/apache/zookeeper/pull/1950#discussion_r1128832116
##########
zookeeper-server/src/main/java/org/apache/zookeeper/WatchedEvent.java:
##########
@@ -66,9 +78,44 @@ public String getPath() {
return path;
}
+ /**
+ * Returns the zxid of the transaction that triggered this watch if it is
+ * of one of the following types:<ul>
+ * <li>{@link EventType#NodeCreated}</li>
+ * <li>{@link EventType#NodeDeleted}</li>
+ * <li>{@link EventType#NodeDataChanged}</li>
+ * <li>{@link EventType#NodeChildrenChanged}</li>
+ * </ul>
+ * Otherwise, returns {@value #NO_ZXID}. Note that {@value #NO_ZXID} is also
+ * returned by old servers that do not support this feature.
+ */
+ public long getZxid() {
+ return zxid;
+ }
+
@Override
public String toString() {
- return "WatchedEvent state:" + keeperState + " type:" + eventType + " path:" + path;
+ return "WatchedEvent state:" + keeperState + " type:" + eventType + " path:" + path + " zxid: " + zxid;
+ }
+
+ @Override
+ public boolean equals(Object o) {
Review Comment:
This could break codes. If we are intend to do so, it might be better a separated jira.
##########
zookeeper-server/src/test/java/org/apache/zookeeper/server/watch/WatchManagerTest.java:
##########
@@ -416,6 +416,10 @@ private void checkMetrics(String metricName, long min, long max, double avg, lon
assertEquals(sum, values.get("sum_" + metricName));
}
+ private void checkWatchedZxid(DumbWatcher watcher, long expectedZxid) {
+ assertEquals(expectedZxid, watcher.getWatchedZxid());
+ }
+
@ParameterizedTest
@MethodSource("data")
public void testWatcherMetrics(String className) throws IOException {
Review Comment:
We are testing more than metrics now. If you are testing zxid, then how about whole event as did in client side tests ? I saw no such assertions in server side for `WatchManager`.
##########
zookeeper-server/src/test/java/org/apache/zookeeper/server/watch/WatchManagerTest.java:
##########
@@ -430,28 +434,37 @@ public void testWatcherMetrics(String className) throws IOException {
final String path3 = "/path3";
- //both wather1 and wather2 are watching path1
+ //both watcher1 and watcher2 are watching path1
manager.addWatch(path1, watcher1);
manager.addWatch(path1, watcher2);
//path2 is watched by watcher1
manager.addWatch(path2, watcher1);
- manager.triggerWatch(path3, EventType.NodeCreated);
+ manager.triggerWatch(path3, EventType.NodeCreated, 1);
//path3 is not being watched so metric is 0
checkMetrics("node_created_watch_count", 0L, 0L, 0D, 0L, 0L);
+ // Watchers shouldn't have received any events yet so the zxid should be -1.
+ checkWatchedZxid(watcher1, -1);
+ checkWatchedZxid(watcher2, -1);
//path1 is watched by two watchers so two fired
- manager.triggerWatch(path1, EventType.NodeCreated);
+ manager.triggerWatch(path1, EventType.NodeCreated, 2);
checkMetrics("node_created_watch_count", 2L, 2L, 2D, 1L, 2L);
+ checkWatchedZxid(watcher1, 2);
+ checkWatchedZxid(watcher2, 2);
//path2 is watched by one watcher so one fired now total is 3
- manager.triggerWatch(path2, EventType.NodeCreated);
+ manager.triggerWatch(path2, EventType.NodeCreated, 3);
checkMetrics("node_created_watch_count", 1L, 2L, 1.5D, 2L, 3L);
+ checkWatchedZxid(watcher1, 3);
+ checkWatchedZxid(watcher2, 2);
//watches on path1 are no longer there so zero fired
- manager.triggerWatch(path1, EventType.NodeDataChanged);
+ manager.triggerWatch(path1, EventType.NodeDataChanged, 4);
checkMetrics("node_changed_watch_count", 0L, 0L, 0D, 0L, 0L);
+ checkWatchedZxid(watcher1, 3);
+ checkWatchedZxid(watcher2, 2);
//both wather1 and wather2 are watching path1
Review Comment:
Typos too.
--
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
[GitHub] [zookeeper] anmolnar commented on pull request #1950: ZOOKEEPER-4655: Communicate the Zxid that triggered a WatchEvent to fire
Posted by "anmolnar (via GitHub)" <gi...@apache.org>.
anmolnar commented on PR #1950:
URL: https://github.com/apache/zookeeper/pull/1950#issuecomment-1593729513
@PapaCharlie Please assign the jira ticket to yourself and set the status to patch available:
https://issues.apache.org/jira/browse/ZOOKEEPER-4655
--
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
[GitHub] [zookeeper] kezhuw commented on pull request #1950: ZOOKEEPER-4655: Communicate the Zxid that triggered a WatchEvent to fire
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on PR #1950:
URL: https://github.com/apache/zookeeper/pull/1950#issuecomment-1558569351
@PapaCharlie I think you can send an email to dev mailing list to ask for opinions and reviews. pr has less exposure chance if it is not get attentions from its creation. I did [once](https://lists.apache.org/thread/ym1vhnv07dtnysoyjrbfrvdjdbfmlzbb) and saw [others](https://lists.apache.org/thread/gnw4czwvk8f5ttd8r9kvdqhml65z7j6g). But before doing that, I recommend you to filter out #1989 related changes to make this pr more concentrated.
--
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
[GitHub] [zookeeper] PapaCharlie commented on pull request #1950: Communicate the Zxid that triggered a WatchEvent to fire
Posted by GitBox <gi...@apache.org>.
PapaCharlie commented on PR #1950:
URL: https://github.com/apache/zookeeper/pull/1950#issuecomment-1335638463
Sorry I can't open a ticket for this. I emailed the mailing list already
--
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
[GitHub] [zookeeper] PapaCharlie commented on a diff in pull request #1950: ZOOKEEPER-4655: Communicate the Zxid that triggered a WatchEvent to fire
Posted by "PapaCharlie (via GitHub)" <gi...@apache.org>.
PapaCharlie commented on code in PR #1950:
URL: https://github.com/apache/zookeeper/pull/1950#discussion_r1128985479
##########
zookeeper-server/src/test/java/org/apache/zookeeper/server/watch/WatchManagerTest.java:
##########
@@ -416,6 +416,10 @@ private void checkMetrics(String metricName, long min, long max, double avg, lon
assertEquals(sum, values.get("sum_" + metricName));
}
+ private void checkWatchedZxid(DumbWatcher watcher, long expectedZxid) {
+ assertEquals(expectedZxid, watcher.getWatchedZxid());
+ }
+
@ParameterizedTest
@MethodSource("data")
public void testWatcherMetrics(String className) throws IOException {
Review Comment:
You're right, this is worth testing. I've updated it, let me know what you think!
--
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
[GitHub] [zookeeper] PapaCharlie commented on a diff in pull request #1950: ZOOKEEPER-4655: Communicate the Zxid that triggered a WatchEvent to fire
Posted by "PapaCharlie (via GitHub)" <gi...@apache.org>.
PapaCharlie commented on code in PR #1950:
URL: https://github.com/apache/zookeeper/pull/1950#discussion_r1128921566
##########
zookeeper-server/src/main/java/org/apache/zookeeper/WatchedEvent.java:
##########
@@ -66,9 +78,44 @@ public String getPath() {
return path;
}
+ /**
+ * Returns the zxid of the transaction that triggered this watch if it is
+ * of one of the following types:<ul>
+ * <li>{@link EventType#NodeCreated}</li>
+ * <li>{@link EventType#NodeDeleted}</li>
+ * <li>{@link EventType#NodeDataChanged}</li>
+ * <li>{@link EventType#NodeChildrenChanged}</li>
+ * </ul>
+ * Otherwise, returns {@value #NO_ZXID}. Note that {@value #NO_ZXID} is also
+ * returned by old servers that do not support this feature.
+ */
+ public long getZxid() {
+ return zxid;
+ }
+
@Override
public String toString() {
- return "WatchedEvent state:" + keeperState + " type:" + eventType + " path:" + path;
+ return "WatchedEvent state:" + keeperState + " type:" + eventType + " path:" + path + " zxid: " + zxid;
+ }
+
+ @Override
+ public boolean equals(Object o) {
Review Comment:
I updated the code and left a TODO
--
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
[GitHub] [zookeeper] PapaCharlie commented on pull request #1950: ZOOKEEPER-4655: Communicate the Zxid that triggered a WatchEvent to fire
Posted by "PapaCharlie (via GitHub)" <gi...@apache.org>.
PapaCharlie commented on PR #1950:
URL: https://github.com/apache/zookeeper/pull/1950#issuecomment-1545559290
Hey @kezhuw, what's the usecase for having a single multi modify the same node more than once? Is there a specific behavior of ZK that is only available via this sort of multi? Based on my understanding of common uses for ZK, it feels like this is a rare usage pattern, so this change is still very useful for anyone that does not do this
--
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
[GitHub] [zookeeper] PapaCharlie commented on pull request #1950: ZOOKEEPER-4655: Communicate the Zxid that triggered a WatchEvent to fire
Posted by "PapaCharlie (via GitHub)" <gi...@apache.org>.
PapaCharlie commented on PR #1950:
URL: https://github.com/apache/zookeeper/pull/1950#issuecomment-1545562228
Also, based on my experience in building a client-side cache, having the zxid in the header would resolve a lot of internal inconsistencies, so ZOOKEEPER-1289 may be at least partially solved :)
--
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
[GitHub] [zookeeper] kezhuw commented on pull request #1950: ZOOKEEPER-4655: Communicate the Zxid that triggered a WatchEvent to fire
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on PR #1950:
URL: https://github.com/apache/zookeeper/pull/1950#issuecomment-1544436457
Yesterday, I have a short out of band (eg. not mailing list) discussion with @tisonkun in a group chat about "why etcd forbid duplicated keys in txn"(https://github.com/etcd-io/etcd/pull/4363, https://github.com/etcd-io/etcd/pull/4376). I think it is partially because of "etcd may not want to deliver multiple mutations on same key with same revision". Then I recalled ZooKeeper side. I wrote a test in https://github.com/kezhuw/zookeeper-client-rust/commit/f6cdacbe4132503b9238ab153c5b845c8ffe369f which mutates one key multiple times and asserts multiple watched events. Sadly, it passed. This basically means that 'zxid' by itself can't not distinguish order of multiple mutations for key in multi (though ordered delivery of watched events are still guaranteed). I guess it make this pr less meaningful. We probably should solve this "multiple mutations for one key in multi op" before continue this pr. But I am afraid of it might be a "won't fix". Anyway, I will open a "Wish" jira later.
cc @tisonkun @eolivelli
--
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
[GitHub] [zookeeper] PapaCharlie commented on a diff in pull request #1950: ZOOKEEPER-4655: Communicate the Zxid that triggered a WatchEvent to fire
Posted by "PapaCharlie (via GitHub)" <gi...@apache.org>.
PapaCharlie commented on code in PR #1950:
URL: https://github.com/apache/zookeeper/pull/1950#discussion_r1204513349
##########
zookeeper-server/src/main/java/org/apache/zookeeper/WatchedEvent.java:
##########
@@ -31,27 +31,38 @@
*/
@InterfaceAudience.Public
public class WatchedEvent {
+ public static final long NO_ZXID = -1L;
private final KeeperState keeperState;
private final EventType eventType;
- private String path;
+ private final String path;
+ private final long zxid;
/**
- * Create a WatchedEvent with specified type, state and path
+ * Create a WatchedEvent with specified type, state, path and zxid
*/
- public WatchedEvent(EventType eventType, KeeperState keeperState, String path) {
+ public WatchedEvent(EventType eventType, KeeperState keeperState, String path, long zxid) {
Review Comment:
This is preserved below, it's just not marked as @deprecated because there are many uses of WatchedEvent that correctly do not have a zxid.
--
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