Details

    • Type: Sub-task Sub-task
    • Status: Resolved
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.5.0
    • Component/s: None
    • Labels:
      None

      Description

      This JIRA to discuss the command line shell for removing watches. Makes it easier to do ad-hoc testing.

      1. ZOOKEEPER-1830.patch
        18 kB
        Rakesh R
      2. ZOOKEEPER-1830.patch
        19 kB
        Rakesh R

        Activity

        Hide
        Hudson added a comment -

        SUCCESS: Integrated in ZooKeeper-trunk #2279 (See https://builds.apache.org/job/ZooKeeper-trunk/2279/)
        ZOOKEEPER-1830. Support command line shell for removing watches (Rakesh R via michim) (michim: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1584454)

        • /zookeeper/trunk/CHANGES.txt
        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/ZooKeeperMain.java
        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/cli/RemoveWatchesCommand.java
        • /zookeeper/trunk/src/java/test/org/apache/zookeeper/RemoveWatchesCmdTest.java
        Show
        Hudson added a comment - SUCCESS: Integrated in ZooKeeper-trunk #2279 (See https://builds.apache.org/job/ZooKeeper-trunk/2279/ ) ZOOKEEPER-1830 . Support command line shell for removing watches (Rakesh R via michim) (michim: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1584454 ) /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/src/java/main/org/apache/zookeeper/ZooKeeperMain.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/cli/RemoveWatchesCommand.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/RemoveWatchesCmdTest.java
        Show
        Michi Mutsuzaki added a comment - trunk: http://svn.apache.org/viewvc?view=revision&revision=1584454
        Hide
        Michi Mutsuzaki added a comment -

        +1 Thank you Rakesh!

        Show
        Michi Mutsuzaki added a comment - +1 Thank you Rakesh!
        Hide
        Rakesh R added a comment -

        Thanks again Raul Gutierrez Segales for the review and your time.

        Hi Patrick Hunt, Please have a look at it and would like to know your thoughts.

        Show
        Rakesh R added a comment - Thanks again Raul Gutierrez Segales for the review and your time. Hi Patrick Hunt , Please have a look at it and would like to know your thoughts.
        Hide
        Raul Gutierrez Segales added a comment -

        lgtm, +1.

        Show
        Raul Gutierrez Segales added a comment - lgtm, +1.
        Hide
        Rakesh R added a comment -

        Thanks Raul Gutierrez Segales for the review. Attached latest patch addressing the comments.

        Also done the following changes apart from the comments:

        1. modified the command name to 'removewatches', I feel this is better inline with the zk#removeWatches api name. Earlier I used 'deletewatches'
        2. moved the unit testing to a new class
        3. added one more test case for default options
        4. modified the test case assetions a bit
        Show
        Rakesh R added a comment - Thanks Raul Gutierrez Segales for the review. Attached latest patch addressing the comments. Also done the following changes apart from the comments: modified the command name to 'removewatches', I feel this is better inline with the zk#removeWatches api name. Earlier I used 'deletewatches' moved the unit testing to a new class added one more test case for default options modified the test case assetions a bit
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12625182/ZOOKEEPER-1830.patch
        against trunk revision 1561236.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 45 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 passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1898//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1898//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1898//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12625182/ZOOKEEPER-1830.patch against trunk revision 1561236. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 45 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 passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1898//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1898//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1898//console This message is automatically generated.
        Hide
        Raul Gutierrez Segales added a comment -

        Nit:

        +        boolean local = false;
        +        if (cl.hasOption("l")) {
        +            local = true;
        +        }
        

        simplify to:

        +        boolean local = cl.hasOption("l");
        

        Nit:

        +        Assert.assertEquals("Didn't recieves DataWatchRemoved!", 1,
        +                pathVsEvent.size());
        +        Assert.assertEquals("Didn't recieves DataWatchRemoved!",
        

        typos: "Didn't receive DataWatchRemoved!".

        And some more:

        +        Assert.assertEquals("Didn't recieves ChildWatchRemoved!", 1,
        +                pathVsEvent.size());
        +        Assert.assertEquals("Didn't recieves ChildWatchRemoved!",
        

        "Didn't receive ...". (there are more instance of "recieves" below).

        Nit:

        +
        +            @Override
        +            public void process(WatchedEvent event) {
        +                switch (event.getType()) {
        +                case ChildWatchRemoved: {
        +                    addWatchNotifications(pathVsEvent, event);
        +                    watcherLatch.countDown();
        +                    break;
        +                }
        +                case DataWatchRemoved: {
        +                    addWatchNotifications(pathVsEvent, event);
        +                    watcherLatch.countDown();
        +                    break;
        +                }
        +                case NodeChildrenChanged: {
        +                    addWatchNotifications(pathVsEvent, event);
        +                    break;
        +                }
        +                case NodeDataChanged: {
        +                    addWatchNotifications(pathVsEvent, event);
        +                    break;
        +                }
        +                }
        +            }
        

        You can group some cases:

        +
        +            @Override
        +            public void process(WatchedEvent event) {
        +                switch (event.getType()) {
        +                case ChildWatchRemoved: 
        +                case DataWatchRemoved: {
        +                    addWatchNotifications(pathVsEvent, event);
        +                    watcherLatch.countDown();
        +                    break;
        +                }
        +                case NodeChildrenChanged:
        +                case NodeDataChanged: {
        +                    addWatchNotifications(pathVsEvent, event);
        +                    break;
        +                }
        +                }
        +            }
        
        Show
        Raul Gutierrez Segales added a comment - Nit: + boolean local = false; + if (cl.hasOption("l")) { + local = true; + } simplify to: + boolean local = cl.hasOption("l"); Nit: + Assert.assertEquals("Didn't recieves DataWatchRemoved!", 1, + pathVsEvent.size()); + Assert.assertEquals("Didn't recieves DataWatchRemoved!", typos: "Didn't receive DataWatchRemoved!". And some more: + Assert.assertEquals("Didn't recieves ChildWatchRemoved!", 1, + pathVsEvent.size()); + Assert.assertEquals("Didn't recieves ChildWatchRemoved!", "Didn't receive ...". (there are more instance of "recieves" below). Nit: + + @Override + public void process(WatchedEvent event) { + switch (event.getType()) { + case ChildWatchRemoved: { + addWatchNotifications(pathVsEvent, event); + watcherLatch.countDown(); + break; + } + case DataWatchRemoved: { + addWatchNotifications(pathVsEvent, event); + watcherLatch.countDown(); + break; + } + case NodeChildrenChanged: { + addWatchNotifications(pathVsEvent, event); + break; + } + case NodeDataChanged: { + addWatchNotifications(pathVsEvent, event); + break; + } + } + } You can group some cases: + + @Override + public void process(WatchedEvent event) { + switch (event.getType()) { + case ChildWatchRemoved: + case DataWatchRemoved: { + addWatchNotifications(pathVsEvent, event); + watcherLatch.countDown(); + break; + } + case NodeChildrenChanged: + case NodeDataChanged: { + addWatchNotifications(pathVsEvent, event); + break; + } + } + }
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12617166/ZOOKEEPER-1830.patch
        against trunk revision 1561236.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 34 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 passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1897//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1897//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1897//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12617166/ZOOKEEPER-1830.patch against trunk revision 1561236. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 34 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 passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1897//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1897//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1897//console This message is automatically generated.
        Hide
        Rakesh R added a comment -

        Could you please have a look at the attached patch.
        This should be applied on top of ZOOKEEPER-442(as this contains the actual client api implementations)

        Delete watches command syntax :

             deletewatches path [-c|-d|-a] [-l]
        
             c - represents childwatchers
             d - represents datawatchers
             a - represents both child/data watchers
             l - represents remove watchers locally when no server connection
        
        Show
        Rakesh R added a comment - Could you please have a look at the attached patch. This should be applied on top of ZOOKEEPER-442 (as this contains the actual client api implementations) Delete watches command syntax : deletewatches path [-c|-d|-a] [-l] c - represents childwatchers d - represents datawatchers a - represents both child/data watchers l - represents remove watchers locally when no server connection

          People

          • Assignee:
            Rakesh R
            Reporter:
            Rakesh R
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development