ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-1350

Please make JMX registration optional in LearnerZooKeeperServer

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 3.4.0
    • Fix Version/s: 3.5.0
    • Component/s: server
    • Labels:
      None

      Description

      LearnerZooKeeperServer has no option to disable JMX registrations. Curator has a test ZK server cluster. Due to the intricacies of JMX, the registrations cannot be easily undone. In order for the Curator Test cluster to be re-usable in a testing session, JavaAssist ugliness was necessary to make LearnerZooKeeperServer.registerJMX() and LearnerZooKeeperServer.unregisterJMX() NOPs.

      I suggest a simple System property.

      1. patch.txt
        19 kB
        Jordan Zimmerman
      2. jmx_optional.diff
        4 kB
        Jordan Zimmerman

        Activity

        Jordan Zimmerman created issue -
        Hide
        Jordan Zimmerman added a comment -

        Patch file

        Show
        Jordan Zimmerman added a comment - Patch file
        Jordan Zimmerman made changes -
        Field Original Value New Value
        Attachment jmx_optional.diff [ 12509347 ]
        Hide
        Jordan Zimmerman added a comment -

        Here's a possible solution that would work for Curator.

        Show
        Jordan Zimmerman added a comment - Here's a possible solution that would work for Curator.
        Jordan Zimmerman made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12509347/jmx_optional.diff
        against trunk revision 1225927.

        +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/875//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/875//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/875//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/12509347/jmx_optional.diff against trunk revision 1225927. +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/875//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/875//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/875//console This message is automatically generated.
        Patrick Hunt made changes -
        Assignee Jordan Zimmerman [ randgalt ]
        Hide
        Patrick Hunt added a comment -

        Jordan, thanks for this patch! Some course grain f/b:

        1) use Boolean.getBoolean in allowJMX
        http://docs.oracle.com/javase/6/docs/api/java/lang/Boolean.html#getBoolean(java.lang.String)

        2) can you update all the jmx registrations, rather than the single class?

        3) we should have a test that verifies both the disable and default case. This should be easy given it's a system property.

        Also, when you resubmit would you mind creating a review on https://reviews.apache.org/dashboard/ ? This script might help:
        https://github.com/phunt/apache-reviewboard-zk-git

        Regards.

        Show
        Patrick Hunt added a comment - Jordan, thanks for this patch! Some course grain f/b: 1) use Boolean.getBoolean in allowJMX http://docs.oracle.com/javase/6/docs/api/java/lang/Boolean.html#getBoolean(java.lang.String ) 2) can you update all the jmx registrations, rather than the single class? 3) we should have a test that verifies both the disable and default case. This should be easy given it's a system property. Also, when you resubmit would you mind creating a review on https://reviews.apache.org/dashboard/ ? This script might help: https://github.com/phunt/apache-reviewboard-zk-git Regards.
        Patrick Hunt made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        Hide
        Jordan Zimmerman added a comment -

        More complete implementation with test

        Show
        Jordan Zimmerman added a comment - More complete implementation with test
        Jordan Zimmerman made changes -
        Attachment patch.txt [ 12509655 ]
        Jordan Zimmerman made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/3410/
        -----------------------------------------------------------

        (Updated 2012-01-06 04:59:53.911307)

        Review request for zookeeper.

        Summary (updated)
        -------

        LearnerZooKeeperServer has no option to disable JMX registrations. Curator has a test ZK server cluster. Due to the intricacies of JMX, the registrations cannot be easily undone. In order for the Curator Test cluster to be re-usable in a testing session, JavaAssist ugliness was necessary to make LearnerZooKeeperServer.registerJMX() and LearnerZooKeeperServer.unregisterJMX() NOPs.

        This addresses bug ZOOKEEPER-1350.
        https://issues.apache.org/jira/browse/ZOOKEEPER-1350

        Diffs


        Diff: https://reviews.apache.org/r/3410/diff

        Testing
        -------

        Thanks,

        Jordan

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3410/ ----------------------------------------------------------- (Updated 2012-01-06 04:59:53.911307) Review request for zookeeper. Summary (updated) ------- LearnerZooKeeperServer has no option to disable JMX registrations. Curator has a test ZK server cluster. Due to the intricacies of JMX, the registrations cannot be easily undone. In order for the Curator Test cluster to be re-usable in a testing session, JavaAssist ugliness was necessary to make LearnerZooKeeperServer.registerJMX() and LearnerZooKeeperServer.unregisterJMX() NOPs. This addresses bug ZOOKEEPER-1350 . https://issues.apache.org/jira/browse/ZOOKEEPER-1350 Diffs Diff: https://reviews.apache.org/r/3410/diff Testing ------- Thanks, Jordan
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/3410/
        -----------------------------------------------------------

        (Updated 2012-01-06 05:02:48.623412)

        Review request for zookeeper.

        Summary
        -------

        LearnerZooKeeperServer has no option to disable JMX registrations. Curator has a test ZK server cluster. Due to the intricacies of JMX, the registrations cannot be easily undone. In order for the Curator Test cluster to be re-usable in a testing session, JavaAssist ugliness was necessary to make LearnerZooKeeperServer.registerJMX() and LearnerZooKeeperServer.unregisterJMX() NOPs.

        This addresses bug ZOOKEEPER-1350.
        https://issues.apache.org/jira/browse/ZOOKEEPER-1350

        Diffs (updated)


        /src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java 1227917
        /src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java 1227917
        /src/java/main/org/apache/zookeeper/server/quorum/AuthFastLeaderElection.java 1227917
        /src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java 1227917
        /src/java/main/org/apache/zookeeper/server/quorum/LeaderElection.java 1227917
        /src/java/main/org/apache/zookeeper/server/quorum/LeaderZooKeeperServer.java 1227917
        /src/java/main/org/apache/zookeeper/server/quorum/LearnerZooKeeperServer.java 1227917
        /src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1227917
        /src/java/main/org/apache/zookeeper/server/quorum/ReadOnlyZooKeeperServer.java 1227917
        /src/java/test/org/apache/zookeeper/server/quorum/LearnerTest.java 1227917

        Diff: https://reviews.apache.org/r/3410/diff

        Testing
        -------

        Thanks,

        Jordan

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3410/ ----------------------------------------------------------- (Updated 2012-01-06 05:02:48.623412) Review request for zookeeper. Summary ------- LearnerZooKeeperServer has no option to disable JMX registrations. Curator has a test ZK server cluster. Due to the intricacies of JMX, the registrations cannot be easily undone. In order for the Curator Test cluster to be re-usable in a testing session, JavaAssist ugliness was necessary to make LearnerZooKeeperServer.registerJMX() and LearnerZooKeeperServer.unregisterJMX() NOPs. This addresses bug ZOOKEEPER-1350 . https://issues.apache.org/jira/browse/ZOOKEEPER-1350 Diffs (updated) /src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java 1227917 /src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java 1227917 /src/java/main/org/apache/zookeeper/server/quorum/AuthFastLeaderElection.java 1227917 /src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java 1227917 /src/java/main/org/apache/zookeeper/server/quorum/LeaderElection.java 1227917 /src/java/main/org/apache/zookeeper/server/quorum/LeaderZooKeeperServer.java 1227917 /src/java/main/org/apache/zookeeper/server/quorum/LearnerZooKeeperServer.java 1227917 /src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1227917 /src/java/main/org/apache/zookeeper/server/quorum/ReadOnlyZooKeeperServer.java 1227917 /src/java/test/org/apache/zookeeper/server/quorum/LearnerTest.java 1227917 Diff: https://reviews.apache.org/r/3410/diff Testing ------- Thanks, Jordan
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12509655/patch.txt
        against trunk revision 1227927.

        +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 passed core unit tests.

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

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/882//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/882//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/882//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/12509655/patch.txt against trunk revision 1227927. +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 passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/882//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/882//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/882//console This message is automatically generated.
        Mahadev konar made changes -
        Fix Version/s 3.5.0 [ 12316644 ]
        Hide
        Michi Mutsuzaki added a comment -

        The patch doesn't apply anymore. Pat, could you take a look when the patch gets rebased?

        Thanks!
        --Michi

        Show
        Michi Mutsuzaki added a comment - The patch doesn't apply anymore. Pat, could you take a look when the patch gets rebased? Thanks! --Michi
        Michi Mutsuzaki made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]

          People

          • Assignee:
            Jordan Zimmerman
            Reporter:
            Jordan Zimmerman
          • Votes:
            1 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:

              Development