Details
-
New Feature
-
Status: Resolved
-
Major
-
Resolution: Fixed
-
None
Description
This is equivalent for ZOOKEEPER-442's Java impl.
Attachments
Attachments
- ZOOKEEPER-1887.patch
- 16 kB
- Raúl Gutiérrez Segalés
- ZOOKEEPER-1887.patch
- 19 kB
- Raúl Gutiérrez Segalés
- ZOOKEEPER-1887.patch
- 21 kB
- Raúl Gutiérrez Segalés
Issue Links
- is related to
-
ZOOKEEPER-1919 Update the C implementation of removeWatches to have it match ZOOKEEPER-1910
- Closed
-
ZOOKEEPER-2611 zoo_remove_watchers - can remove the wrong watch
- Closed
-
ZOOKEEPER-1829 Umbrella jira for removing watches that are no longer of interest
- Resolved
- relates to
-
ZOOKEEPER-442 need a way to remove watches that are no longer of interest
- Resolved
Activity
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12630591/ZOOKEEPER-1887.patch
against trunk revision 1569590.
+1 @author. The patch does not contain any @author tags.
-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed core unit tests.
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1931//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1931//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1931//console
This message is automatically generated.
Added (very) basic tests and fixed a bug when cleaning up removed watchers.
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12630596/ZOOKEEPER-1887.patch
against trunk revision 1569590.
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 3 new or modified tests.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
-1 core tests. The patch failed core unit tests.
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1932//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1932//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1932//console
This message is automatically generated.
Heads-up for reviewers. I think there are two things I might want to change:
- instead of just being able to remove a watcher via its watcher_fn, we might want to support removing by the (watcher_fn, context) tuple as well. Does it sound useful?
- watcher_registration_t and watcher_deregistration_t are similar enough that we could have a union call watcher_action_t of those two and consolidate all the methods that manipulate them (i.e.: creating a
{,de}
registration, activating, deactivating and destroying). It'll reduce a bit the similar code and perhaps make it a bit more readable. But I'll wait for feedback before making this change.
This feature is a bit more complicated that what I have thought. I will try to spend more time understanding Java implementation so I can review this one more thoroughly.
Here is some of comment on my first pass on the patch
- There is startServer() and stopserver() facility in TestClient.cc. I wondering if you can use this to expand the test coverage for the feature especially for local=true. Since you can remove the watch even if you are in disconnected state
- In c-client a watch is represented by (watcher_fn, context) tuple or watcher_object_t internally. To make the c-client has the equivalent functionality with Java client, we have to actually remove a watch based on (watcher_fn, context) tuple instead. What do you think?
Thanks for the review thawan.
There is startServer() and stopserver() facility in TestClient.cc. I wondering if you can use this to expand the test coverage for the feature especially for local=true. Since you can remove the watch even if you are in disconnected state
sure - I'll add that to the case in which I test local=true after calling zk.close().
In c-client a watch is represented by (watcher_fn, context) tuple or watcher_object_t internally. To make the c-client has the equivalent functionality with Java client, we have to actually remove a watch based on (watcher_fn, context) tuple instead. What do you think?
yeah i think maybe both ways would be convenient? i.e. you can either remove anything that matches a watcher_fn or a specific tuple (watcher_fn, context). Not sure if we need to support both.
I don't think we need to support both, but we need to support (watcher_fn, context) to make it equivalent to Java client.
Great to see this being worked on, keeping parity btw java and c clients would be nice. Kudos!
ps. you might also want to expose this in zkpython once you resolve this jira, i've found it's a great way to exercise the underlying code, not to mention helpful for folks using python.
Raul, could you update the patch to address Thawan's feedback? I reviewed the patch, and it looks good overall.
Hey michim - yes will get it updated this weekend (sorry, got distracted).
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12639120/ZOOKEEPER-1887.patch
against trunk revision 1585371.
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 3 new or modified tests.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
-1 core tests. The patch failed core unit tests.
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2028//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2028//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2028//console
This message is automatically generated.
Hmm, so failed tests say:
[exec] [exec] /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-Build/trunk/src/c/tests/TestWatchers.cc:667: Assertion: assertion failed [Expression: ensureCondition( deliveryTracker.deliveryCounterEquals(2),1000)<1000] [exec] [exec] Failures !!! [exec] [exec] Run: 71 Failure total: 1 Failures: 1 Errors: 0 [exec] [exec] FAIL: zktest-mt [exec] [exec] ========================================== [exec] [exec] 1 of 2 tests failed [exec] [exec] Please report to user@zookeeper.apache.org [exec] [exec] ==========================================
Though, from my checkout:
$ ./zktest-mt Zookeeper_watchers localhost:2181 ZooKeeper server startedRunning Zookeeper_watchers::testDefaultSessionWatcher1 : elapsed 53 : OK Zookeeper_watchers::testDefaultSessionWatcher2 : elapsed 4 : OK Zookeeper_watchers::testObjectSessionWatcher1 : elapsed 54 : OK Zookeeper_watchers::testObjectSessionWatcher2 : elapsed 56 : OK Zookeeper_watchers::testNodeWatcher1 : elapsed 57 : OK Zookeeper_watchers::testChildWatcher1 : elapsed 53 : OK Zookeeper_watchers::testChildWatcher2 : elapsed 53 : OK OK (7)
Are these tests known to be flaky?
michim: did you get a chance to look at the updated patch? OTOH, I've been running the C tests on my box and a combination of them fail at different times so not sure about Jenkins unhappiness. Some of them just seem to have small timeouts...
Raul, I'll try to review the patch tomorrow. I restarted the pre-commit build in the meantime to see if the failure persists.
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12639120/ZOOKEEPER-1887.patch
against trunk revision 1585371.
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 3 new or modified tests.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
-1 core tests. The patch failed core unit tests.
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2029//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2029//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2029//console
This message is automatically generated.
trunk: http://svn.apache.org/viewvc?view=revision&revision=1587812
Thanks Raul!
FAILURE: Integrated in ZooKeeper-trunk #2292 (See https://builds.apache.org/job/ZooKeeper-trunk/2292/)
ZOOKEEPER-1887. C implementation of removeWatches (Raul Gutierrez Segales via michim) (michim: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1587812)
- /zookeeper/trunk/CHANGES.txt
- /zookeeper/trunk/src/c/include/proto.h
- /zookeeper/trunk/src/c/include/zookeeper.h
- /zookeeper/trunk/src/c/src/zk_hashtable.c
- /zookeeper/trunk/src/c/src/zk_hashtable.h
- /zookeeper/trunk/src/c/src/zookeeper.c
- /zookeeper/trunk/src/c/tests/TestClient.cc
No tests yet, will add them later this week.