ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-1350

Please make JMX registration optional in LearnerZooKeeperServer

    Details

    • Type: Improvement Improvement
    • Status: Patch Available
    • 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. ZOOKEEPER-1350-trunk.diff
        40 kB
        Stevo Slavic
      2. ZOOKEEPER-1350.diff
        40 kB
        Stevo Slavic
      3. patch.txt
        19 kB
        Jordan Zimmerman
      4. jmx_optional.diff
        4 kB
        Jordan Zimmerman

        Activity

        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12650322/ZOOKEEPER-1350-trunk.diff
        against trunk revision 1601516.

        +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 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2135//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/12650322/ZOOKEEPER-1350-trunk.diff against trunk revision 1601516. +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 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2135//console This message is automatically generated.
        Stevo Slavic made changes -
        Attachment ZOOKEEPER-1350-trunk.diff [ 12650322 ]
        Hide
        Stevo Slavic added a comment -

        Attaching ZOOKEEPER-1350-trunk.diff.
        Compared to ZOOKEEPER-1350.diff, this one can be applied to trunk branch + it contains tiny improvement as suggested by Raúl Gutiérrez Segalés in his comment on pull request.

        Show
        Stevo Slavic added a comment - Attaching ZOOKEEPER-1350-trunk.diff . Compared to ZOOKEEPER-1350.diff , this one can be applied to trunk branch + it contains tiny improvement as suggested by Raúl Gutiérrez Segalés in his comment on pull request .
        Hide
        ASF GitHub Bot added a comment -

        GitHub user sslavic opened a pull request:

        https://github.com/apache/zookeeper/pull/14

        Make JMX registration optional

        This patch is a modification of patch.txt, which is attached to ZOOKEEPER-1350, that can be applied to "trunk" branch.

        You can merge this pull request into a Git repository by running:

        $ git pull https://github.com/sslavic/zookeeper ZOOKEEPER-1350-trunk

        Alternatively you can review and apply these changes as the patch at:

        https://github.com/apache/zookeeper/pull/14.patch

        To close this pull request, make a commit to your master/trunk branch
        with (at least) the following in the commit message:

        This closes #14


        commit d24041b6443a3979134917e44fcb5a8e29b47f42
        Author: Stevo Slavic <sslavic@gmail.com>
        Date: 2014-06-13T15:36:02Z

        ZOOKEEPER-1350 Made JMX registration optional


        Show
        ASF GitHub Bot added a comment - GitHub user sslavic opened a pull request: https://github.com/apache/zookeeper/pull/14 Make JMX registration optional This patch is a modification of patch.txt, which is attached to ZOOKEEPER-1350 , that can be applied to "trunk" branch. You can merge this pull request into a Git repository by running: $ git pull https://github.com/sslavic/zookeeper ZOOKEEPER-1350 -trunk Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/14.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #14 commit d24041b6443a3979134917e44fcb5a8e29b47f42 Author: Stevo Slavic <sslavic@gmail.com> Date: 2014-06-13T15:36:02Z ZOOKEEPER-1350 Made JMX registration optional
        Hide
        Stevo Slavic added a comment -

        I've just created a pull request too, with same changes (see https://github.com/apache/zookeeper/pull/13).

        Show
        Stevo Slavic added a comment - I've just created a pull request too, with same changes (see https://github.com/apache/zookeeper/pull/13 ).
        Hide
        ASF GitHub Bot added a comment -

        GitHub user sslavic opened a pull request:

        https://github.com/apache/zookeeper/pull/13

        Make JMX registration optional

        This patch is a modification of [patch.txt](https://issues.apache.org/jira/secure/attachment/12509655/patch.txt), which is attached to ZOOKEEPER-1350(https://issues.apache.org/jira/browse/ZOOKEEPER-1350), that can be applied to branch "branch-3.4".

        You can merge this pull request into a Git repository by running:

        $ git pull https://github.com/sslavic/zookeeper ZOOKEEPER-1350

        Alternatively you can review and apply these changes as the patch at:

        https://github.com/apache/zookeeper/pull/13.patch

        To close this pull request, make a commit to your master/trunk branch
        with (at least) the following in the commit message:

        This closes #13


        commit f53accafc05b50df6e848a1e6a1494a71904cc79
        Author: Stevo Slavic <sslavic@gmail.com>
        Date: 2014-06-13T12:23:38Z

        ZOOKEEPER-1350 Made JMX registration optional


        Show
        ASF GitHub Bot added a comment - GitHub user sslavic opened a pull request: https://github.com/apache/zookeeper/pull/13 Make JMX registration optional This patch is a modification of [patch.txt] ( https://issues.apache.org/jira/secure/attachment/12509655/patch.txt ), which is attached to ZOOKEEPER-1350 ( https://issues.apache.org/jira/browse/ZOOKEEPER-1350 ), that can be applied to branch "branch-3.4". You can merge this pull request into a Git repository by running: $ git pull https://github.com/sslavic/zookeeper ZOOKEEPER-1350 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/13.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #13 commit f53accafc05b50df6e848a1e6a1494a71904cc79 Author: Stevo Slavic <sslavic@gmail.com> Date: 2014-06-13T12:23:38Z ZOOKEEPER-1350 Made JMX registration optional
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12650298/ZOOKEEPER-1350.diff
        against trunk revision 1601516.

        +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 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2134//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/12650298/ZOOKEEPER-1350.diff against trunk revision 1601516. +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 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2134//console This message is automatically generated.
        Stevo Slavic made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Stevo Slavic made changes -
        Attachment ZOOKEEPER-1350.diff [ 12650298 ]
        Hide
        Stevo Slavic added a comment -

        Attaching ZOOKEEPER-1350.diff - it's a modification of patch.txt so it can be applied to branch-3.4.

        Show
        Stevo Slavic added a comment - Attaching ZOOKEEPER-1350.diff - it's a modification of patch.txt so it can be applied to branch-3.4 .
        Michi Mutsuzaki made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        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
        Mahadev konar made changes -
        Fix Version/s 3.5.0 [ 12316644 ]
        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.
        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
        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
        Jordan Zimmerman made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Jordan Zimmerman made changes -
        Attachment patch.txt [ 12509655 ]
        Hide
        Jordan Zimmerman added a comment -

        More complete implementation with test

        Show
        Jordan Zimmerman added a comment - More complete implementation with test
        Patrick Hunt made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        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 -
        Assignee Jordan Zimmerman [ randgalt ]
        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.
        Jordan Zimmerman made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        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 -
        Field Original Value New Value
        Attachment jmx_optional.diff [ 12509347 ]
        Hide
        Jordan Zimmerman added a comment -

        Patch file

        Show
        Jordan Zimmerman added a comment - Patch file
        Jordan Zimmerman created issue -

          People

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

            Dates

            • Created:
              Updated:

              Development