Details

    • New Feature
    • Status: Resolved
    • Major
    • Resolution: Fixed
    • None
    • 3.5.0
    • c client

    Description

      This is equivalent for ZOOKEEPER-442's Java impl.

      Attachments

        1. ZOOKEEPER-1887.patch
          16 kB
          Raúl Gutiérrez Segalés
        2. ZOOKEEPER-1887.patch
          19 kB
          Raúl Gutiérrez Segalés
        3. ZOOKEEPER-1887.patch
          21 kB
          Raúl Gutiérrez Segalés

        Issue Links

          Activity

            No tests yet, will add them later this week.

            rgs Raúl Gutiérrez Segalés added a comment - No tests yet, will add them later this week.
            hadoopqa Hadoop QA added a comment -

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

            hadoopqa Hadoop QA added a comment - -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.

            rgs Raúl Gutiérrez Segalés added a comment - Added (very) basic tests and fixed a bug when cleaning up removed watchers.
            hadoopqa Hadoop QA added a comment -

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

            hadoopqa Hadoop QA added a comment - -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.

            rgs Raúl Gutiérrez Segalés added a comment - 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?
            thawan Thawan Kooburat added a comment - 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.

            rgs Raúl Gutiérrez Segalés added a comment - 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.

            thawan Thawan Kooburat added a comment - 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.

            phunt Patrick D. Hunt added a comment - 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.

            michim Michi Mutsuzaki added a comment - 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).

            rgs Raúl Gutiérrez Segalés added a comment - Hey michim - yes will get it updated this weekend (sorry, got distracted).

            Addressed thawan's comments. Let me know if there's anything else you'd like merged, michim.

            rgs Raúl Gutiérrez Segalés added a comment - Addressed thawan 's comments. Let me know if there's anything else you'd like merged, michim .
            hadoopqa Hadoop QA added a comment -

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

            hadoopqa Hadoop QA added a comment - -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?

            rgs Raúl Gutiérrez Segalés added a comment - 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...

            rgs Raúl Gutiérrez Segalés added a comment - 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.

            michim Michi Mutsuzaki added a comment - Raul, I'll try to review the patch tomorrow. I restarted the pre-commit build in the meantime to see if the failure persists.
            hadoopqa Hadoop QA added a comment -

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

            hadoopqa Hadoop QA added a comment - -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.

            +1

            thawan, could you also take a look?

            michim Michi Mutsuzaki added a comment - +1 thawan , could you also take a look?

            I'm checking this in.

            michim Michi Mutsuzaki added a comment - I'm checking this in.
            michim Michi Mutsuzaki added a comment - trunk: http://svn.apache.org/viewvc?view=revision&revision=1587812 Thanks Raul!
            hudson Hudson added a comment -

            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
            hudson Hudson added a comment - 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

            People

              rgs Raúl Gutiérrez Segalés
              rgs Raúl Gutiérrez Segalés
              Votes:
              1 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: