Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.4.6
    • Fix Version/s: 3.4.7, 3.5.1, 3.6.0
    • Component/s: server
    • Labels:
      None
    • Environment:

      All

    • Release Note:
      Changes in zkServer.sh to support JMX remote monitoring of Zookeeper processes. The change doesn't impact current installations and new installations requiring JMX remote monitoring need to set the jmx port to enable it.

      Description

      The zooker server start up script includes the option to enable jmx monitoring but only locally. Can we update the script so that remote monitoring can also be enabled which will help in data collection and monitoring through a centralized monitoring tool.

      1. ZOOKEEPER-1948.patch.v2
        1 kB
        Biju Nair
      2. ZOOKEEPER-1948.patch
        1 kB
        Biju Nair
      3. ZOOKEEPER-1948.patch
        1 kB
        Biju Nair
      4. zookeeper-1948.patch
        1 kB
        Biju Nair

        Issue Links

          Activity

          Hide
          nijel added a comment -

          Adding to the suggestion.
          If the remote JMX server is enabled, it will use one Random port. This may be in convenient for cluster managers.
          So i suggest we can have a custom JMX server which is based on RMI

          Refer : https://issues.apache.org/jira/browse/HBASE-10289

          Show
          nijel added a comment - Adding to the suggestion. If the remote JMX server is enabled, it will use one Random port. This may be in convenient for cluster managers. So i suggest we can have a custom JMX server which is based on RMI Refer : https://issues.apache.org/jira/browse/HBASE-10289
          Hide
          Flavio Junqueira added a comment -

          Is this related to ZOOKEEPER-1346?

          Show
          Flavio Junqueira added a comment - Is this related to ZOOKEEPER-1346 ?
          Hide
          nijel added a comment -

          Is this related to ZOOKEEPER-1346?

          From the discussion i understand the issue ZOOKEEPER-1346 is for supporting a new REST interface for 4wls.
          Even after this implementation users will have option to use the remote JMX monitoring by enabling JMX server.

          But if the remote monitoring is enabled, then it will use 2 extra ports, one of which is random.
          So my suggestion was to implement RMI based custom JMX server where the ports can be configured.

          BTW
          this issue (Enable JMX remote monitoring) is because of missing arguement com.sun.management.jmxremote.authenticate=false

          As per code remote monitoring should be enabled since default value of JMXLOCALONLY=false.

          I added this property as -D args and it worked. i think need to add this in script

          Show
          nijel added a comment - Is this related to ZOOKEEPER-1346 ? From the discussion i understand the issue ZOOKEEPER-1346 is for supporting a new REST interface for 4wls. Even after this implementation users will have option to use the remote JMX monitoring by enabling JMX server. But if the remote monitoring is enabled, then it will use 2 extra ports, one of which is random. So my suggestion was to implement RMI based custom JMX server where the ports can be configured. BTW this issue (Enable JMX remote monitoring) is because of missing arguement com.sun.management.jmxremote.authenticate =false As per code remote monitoring should be enabled since default value of JMXLOCALONLY=false. I added this property as -D args and it worked. i think need to add this in script
          Hide
          Biju Nair added a comment -

          Changes in zkServer.sh to support JMX remote monitoring of Zookeeper processes. The change doesn't impact current installations and new installations requiring JMX remote monitoring need to set the jmx port to enable it.

          Show
          Biju Nair added a comment - Changes in zkServer.sh to support JMX remote monitoring of Zookeeper processes. The change doesn't impact current installations and new installations requiring JMX remote monitoring need to set the jmx port to enable it.
          Hide
          Biju Nair added a comment -

          Please review the code changes and let me know if there are any issues. Currently the changed script is used in our development environment.

          Show
          Biju Nair added a comment - Please review the code changes and let me know if there are any issues. Currently the changed script is used in our development environment.
          Hide
          Biju Nair added a comment -

          We should also incorporate ~nigels work on HBASE-10289 which will help configure a port of choice for JMX.

          Show
          Biju Nair added a comment - We should also incorporate ~nigels work on HBASE-10289 which will help configure a port of choice for JMX.
          Hide
          Patrick Hunt added a comment -

          Cancelling the patch while Biju addresses the recent comment (enable the port to be configured)

          Show
          Patrick Hunt added a comment - Cancelling the patch while Biju addresses the recent comment (enable the port to be configured)
          Hide
          Biju Nair added a comment -

          Hi Patrick,
          The change provided in the patch will help most of the users and definitely the one we are currently dealing with. Can we merge the patch while the feature to make the JMX port configurable as in HBASE-10289 is handled seperately?

          Show
          Biju Nair added a comment - Hi Patrick, The change provided in the patch will help most of the users and definitely the one we are currently dealing with. Can we merge the patch while the feature to make the JMX port configurable as in HBASE-10289 is handled seperately?
          Hide
          Patrick Hunt added a comment -

          That's fine. In that case open another jira for that and "link" it to this one (the link jira menu item under "more")

          Show
          Patrick Hunt added a comment - That's fine. In that case open another jira for that and "link" it to this one (the link jira menu item under "more")
          Hide
          Patrick Hunt added a comment -

          Some review feedback:

          1) "JMX enabled by default" needs to be output regardless whether the port is specified or not.

          2) remove the echo "***" lines, not necessary/clutter imo.

          3) Spell ZK correctly, "ZooKeeper"

          4) why do we have

          -Dcom.sun.management.jmxremote.local.only=$JMXLOCALONLY

          if we're specifying remote?

          5) given we're enabling remote, and security is always an issue with this, we should allow these options to also be specified:

          -Dcom.sun.management.jmxremote.authenticate=false -Dcom.sun.management.jmxremote.ssl=false

          Show
          Patrick Hunt added a comment - Some review feedback: 1) "JMX enabled by default" needs to be output regardless whether the port is specified or not. 2) remove the echo "***" lines, not necessary/clutter imo. 3) Spell ZK correctly, "ZooKeeper" 4) why do we have -Dcom.sun.management.jmxremote.local.only=$JMXLOCALONLY if we're specifying remote? 5) given we're enabling remote, and security is always an issue with this, we should allow these options to also be specified: -Dcom.sun.management.jmxremote.authenticate=false -Dcom.sun.management.jmxremote.ssl=false
          Hide
          Patrick Hunt added a comment -

          There should also be some addl detail on the settings for those parameters output to the console. (e.g. what's the auth set to, ssl, etc...)

          Show
          Patrick Hunt added a comment - There should also be some addl detail on the settings for those parameters output to the console. (e.g. what's the auth set to, ssl, etc...)
          Hide
          Biju Nair added a comment -

          Updated patch: Updates are for the review comments.

          Show
          Biju Nair added a comment - Updated patch: Updates are for the review comments.
          Hide
          Biju Nair added a comment -

          Thanks Patrick for the review comments. Attached a new patch file. Let me know if you have any further comments.

          Show
          Biju Nair added a comment - Thanks Patrick for the review comments. Attached a new patch file. Let me know if you have any further comments.
          Hide
          Rakesh R added a comment -

          Thanks Biju Nair for the patch. I've few comments, please see.

          1. Please modify "xJMXSSL" to "x$JMXSSL"
            if [ "xJMXSSL" = "x" ]
            
          2. Any reason to disable zookeeper.jmx.log4j.disable=true?
          3. Minor suggestion - move the following echo msg above to if [ "x$JMXPORT" = "x" ] statement, just to avoid duplicates
            echo "ZooKeeper JMX enabled by default" >&2
            
          Show
          Rakesh R added a comment - Thanks Biju Nair for the patch. I've few comments, please see. Please modify "xJMXSSL" to "x$JMXSSL" if [ "xJMXSSL" = "x" ] Any reason to disable zookeeper.jmx.log4j.disable=true? Minor suggestion - move the following echo msg above to if [ "x$JMXPORT" = "x" ] statement, just to avoid duplicates echo "ZooKeeper JMX enabled by default " >&2
          Hide
          Biju Nair added a comment -

          Updates to the comments from Rakesh.

          Show
          Biju Nair added a comment - Updates to the comments from Rakesh.
          Hide
          Biju Nair added a comment -

          Thanks Rakesh R.

          1) Made changes to the code.
          2) Did add a new varable and code so that users will be able to enable/disable log4j jmx.
          3) Made changes to the code.

          Attached an updated patch (v2). Let me know if there are other commets.

          Show
          Biju Nair added a comment - Thanks Rakesh R. 1) Made changes to the code. 2) Did add a new varable and code so that users will be able to enable/disable log4j jmx. 3) Made changes to the code. Attached an updated patch (v2). Let me know if there are other commets.
          Hide
          Rakesh R added a comment -

          Thanks Biju Nair, it looks better. I prefer to use "ZooKeeper" instead of small letter "Zookeeper" in the echo messages. Sorry to include this in my previous review.

          +    echo "Zookeeper remote JMX ssl set to $JMXSSL" >&2
          +    echo "Zookeeper remote JMX log4j set to $JMXLOG4J" >&2
          

          Secondly, IMHO good to create patch with the JIRA name itself like 'ZOOKEEPER-1948.patch'. Also, press 'Submit Patch' button once the patch is ready to go for review and commit. This will be helpful for the reviewers.

          Show
          Rakesh R added a comment - Thanks Biju Nair , it looks better. I prefer to use "ZooKeeper" instead of small letter "Zookeeper" in the echo messages. Sorry to include this in my previous review. + echo "Zookeeper remote JMX ssl set to $JMXSSL" >&2 + echo "Zookeeper remote JMX log4j set to $JMXLOG4J" >&2 Secondly, IMHO good to create patch with the JIRA name itself like ' ZOOKEEPER-1948 .patch'. Also, press 'Submit Patch' button once the patch is ready to go for review and commit. This will be helpful for the reviewers.
          Hide
          Biju Nair added a comment -

          Changes for the latest review comments.

          Show
          Biju Nair added a comment - Changes for the latest review comments.
          Hide
          Biju Nair added a comment -

          Thanks for the feedback Rakesh. Submitted an updated patch. If required please remove the patch files which doesn't follow the patch file naming conventions.

          Show
          Biju Nair added a comment - Thanks for the feedback Rakesh. Submitted an updated patch. If required please remove the patch files which doesn't follow the patch file naming conventions.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12665829/ZOOKEEPER-1948.patch
          against trunk revision 1621313.

          +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 2.0.3) 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/2306//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2306//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2306//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/12665829/ZOOKEEPER-1948.patch against trunk revision 1621313. +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 2.0.3) 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/2306//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2306//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2306//console This message is automatically generated.
          Hide
          Rakesh R added a comment -

          +1 lgtm.

          Show
          Rakesh R added a comment - +1 lgtm.
          Hide
          Rakesh R added a comment -

          Patrick Hunt Since you had reviewed previously, would like to see your comments on latest patch. Thanks!. Shall I commit this one?

          Show
          Rakesh R added a comment - Patrick Hunt Since you had reviewed previously, would like to see your comments on latest patch. Thanks!. Shall I commit this one?
          Hide
          Rakesh R added a comment -

          It seems the patch is ready to go in and will commit if I get a +1. Thanks!

          Show
          Rakesh R added a comment - It seems the patch is ready to go in and will commit if I get a +1. Thanks!
          Hide
          Michi Mutsuzaki added a comment -

          +1

          Show
          Michi Mutsuzaki added a comment - +1
          Show
          Rakesh R added a comment - Thanks Biju Nair for the patch and Patrick Hunt , Michi Mutsuzaki for the reviews. Committed to trunk : http://svn.apache.org/viewvc?view=revision&revision=1628224 Committed to br3.4 : http://svn.apache.org/viewvc?view=revision&revision=1628225 Committed to br3.5 : http://svn.apache.org/viewvc?view=revision&revision=1628226
          Hide
          Hudson added a comment -

          FAILURE: Integrated in ZooKeeper-trunk #2454 (See https://builds.apache.org/job/ZooKeeper-trunk/2454/)
          ZOOKEEPER-1948 Enable JMX remote monitoring (Biju Nair via rakeshr) (rakeshr: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1628224)

          • /zookeeper/trunk/CHANGES.txt
          • /zookeeper/trunk/bin/zkServer.sh
          Show
          Hudson added a comment - FAILURE: Integrated in ZooKeeper-trunk #2454 (See https://builds.apache.org/job/ZooKeeper-trunk/2454/ ) ZOOKEEPER-1948 Enable JMX remote monitoring (Biju Nair via rakeshr) (rakeshr: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1628224 ) /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/bin/zkServer.sh
          Hide
          Maxim Novikov added a comment -

          The patches seem to support enabling/disabling JMX only through the startup script. However, if you launch ZooKeeper server in runtime from the code as shown below

          ZooKeeperServerMain server = new ZooKeeperServerMain();
          // ...
          server.runFromConfig(serverConfig);
          

          there is still no way to disable JMX as the current JMXDISABLE is handled only when you run ZooKeeper via ZooKeeperServerMain's or QuorumPeerMain's main() methods.

          Show
          Maxim Novikov added a comment - The patches seem to support enabling/disabling JMX only through the startup script. However, if you launch ZooKeeper server in runtime from the code as shown below ZooKeeperServerMain server = new ZooKeeperServerMain(); // ... server.runFromConfig(serverConfig); there is still no way to disable JMX as the current JMXDISABLE is handled only when you run ZooKeeper via ZooKeeperServerMain's or QuorumPeerMain's main() methods.

            People

            • Assignee:
              Biju Nair
              Reporter:
              Biju Nair
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development