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-unsubscr...@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
