ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-1214

QuorumPeer should unregister only its previsously registered MBeans instead of use MBeanRegistry.unregisterAll() method.

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.5.0
    • Component/s: quorum
    • Labels:
      None

      Description

      When a QuorumPeer thread dies, it is unregistering all ZKMBeanInfo MBeans previously registered on its java process; including those that has not been registered by itself.

      It does not cause any side effect in production environment where each server is running on a separate java process; but fails when using "org.apache.zookeeper.test.QuorumUtil" to programmatically start up a zookeeper server ensemble and use its provided methods to force Disconnected, SyncConnected or SessionExpired events; in order to perform some basic/functional testing.

      Scenario:

      • QuorumUtil qU = new QuorumUtil(1); // It creates a 3 servers ensemble.
      • qU.startAll(); // Startup all servers: 1 Leader + 2 Followers
      • qU.shutdown(i); // i is a number from 1 to 3. It shutdown one server.

      The last method causes that a QuorumPeer will die, invoking the MBeanRegistry.unregisterAll() method.
      As a result, all ZKMBeanInfo MBeans are unregistered; including those belonging to the other QuorumPeer instances.

      When trying to restart previous server (qU.restart(i)) an AssertionError is thrown at MBeanRegistry.register(ZKMBeanInfo bean, ZKMBeanInfo parent) method, causing the QuorumPeer thread dead.

      To solve it:

      • MBeanRegistry.unregisterAll() method has been removed.
      • QuorumPeer only unregister its ZKMBeanInfo MBeans.
      1. ZOOKEEPER-1214.patch
        8 kB
        César Álvarez Núñez
      2. ZOOKEEPER-1214.patch
        8 kB
        César Álvarez Núñez
      3. ZOOKEEPER-1214.patch
        10 kB
        César Álvarez Núñez
      4. ZOOKEEPER-1214.patch
        11 kB
        Michi Mutsuzaki
      5. ZOOKEEPER-1214.3.patch
        11 kB
        Edward Ribeiro
      6. ZOOKEEPER-1214.2.patch
        11 kB
        Edward Ribeiro

        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/12497775/ZOOKEEPER-1214.patch
        against trunk revision 1177432.

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

        +1 tests included. The patch appears to include 6 new or modified tests.

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/603//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/12497775/ZOOKEEPER-1214.patch against trunk revision 1177432. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/603//console This message is automatically generated.
        Hide
        César Álvarez Núñez added a comment -

        Refreshed trunk.
        Regenerated patch.

        Show
        César Álvarez Núñez added a comment - Refreshed trunk. Regenerated patch.
        Hide
        Hadoop QA added a comment -

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

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

        +1 tests included. The patch appears to include 6 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/605//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/605//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/605//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/12497782/ZOOKEEPER-1214.patch against trunk revision 1177432. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 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/605//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/605//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/605//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/2208/
        -----------------------------------------------------------

        Review request for zookeeper.

        Summary
        -------

        See https://issues.apache.org/jira/browse/ZOOKEEPER-1214

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

        Diffs


        /src/java/test/org/apache/zookeeper/test/QuorumUtil.java 1179165
        /src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java PRE-CREATION
        /src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1179166
        /src/java/main/org/apache/zookeeper/jmx/MBeanRegistry.java 1169669

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

        Testing
        -------

        -

        Thanks,

        César

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2208/ ----------------------------------------------------------- Review request for zookeeper. Summary ------- See https://issues.apache.org/jira/browse/ZOOKEEPER-1214 This addresses bug ZOOKEEPER-1214 . https://issues.apache.org/jira/browse/ZOOKEEPER-1214 Diffs /src/java/test/org/apache/zookeeper/test/QuorumUtil.java 1179165 /src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java PRE-CREATION /src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1179166 /src/java/main/org/apache/zookeeper/jmx/MBeanRegistry.java 1169669 Diff: https://reviews.apache.org/r/2208/diff Testing ------- - Thanks, César
        Hide
        Patrick Hunt added a comment -

        I updated the RB review with some f/b that needs to be incorporated before this can be committed.

        Show
        Patrick Hunt added a comment - I updated the RB review with some f/b that needs to be incorporated before this can be committed.
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        a good cleanup, some comments before we can commit it.

        /src/java/main/org/apache/zookeeper/jmx/MBeanRegistry.java
        <https://reviews.apache.org/r/2208/#comment8794>

        cleanup this typo (2 returns)

        /src/java/main/org/apache/zookeeper/jmx/MBeanRegistry.java
        <https://reviews.apache.org/r/2208/#comment8793>

        let's call this

        getRegisteredBeans()

        /src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java
        <https://reviews.apache.org/r/2208/#comment8795>

        Add back the try/catch protection. This was necessary in some cases (I can't remember why off the top of my head though).

        /src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java
        <https://reviews.apache.org/r/2208/#comment8796>

        license header is missing. add the std boilerplate found on any other java file in the project.

        /src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java
        <https://reviews.apache.org/r/2208/#comment8798>

        add a comment to the class describing what this set of tests is for

        /src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java
        <https://reviews.apache.org/r/2208/#comment8797>

        no tabs, reformat with spaces only. (you'll probably need to cleanup some of your other changes at the same time)

        /src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java
        <https://reviews.apache.org/r/2208/#comment8799>

        it's fine to ref a jira here, however there should also be some details on what this test is doing. (short summary esp as the test itself is pretty clear)

        • Patrick

        On 2011-10-05 11:59:30, César Álvarez Núñez wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2208/

        -----------------------------------------------------------

        (Updated 2011-10-05 11:59:30)

        Review request for zookeeper.

        Summary

        -------

        See https://issues.apache.org/jira/browse/ZOOKEEPER-1214

        This addresses bug ZOOKEEPER-1214.

        https://issues.apache.org/jira/browse/ZOOKEEPER-1214

        Diffs

        -----

        /src/java/test/org/apache/zookeeper/test/QuorumUtil.java 1179165

        /src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java PRE-CREATION

        /src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1179166

        /src/java/main/org/apache/zookeeper/jmx/MBeanRegistry.java 1169669

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

        Testing

        -------

        -

        Thanks,

        César

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2208/#review3914 ----------------------------------------------------------- a good cleanup, some comments before we can commit it. /src/java/main/org/apache/zookeeper/jmx/MBeanRegistry.java < https://reviews.apache.org/r/2208/#comment8794 > cleanup this typo (2 returns) /src/java/main/org/apache/zookeeper/jmx/MBeanRegistry.java < https://reviews.apache.org/r/2208/#comment8793 > let's call this getRegisteredBeans() /src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java < https://reviews.apache.org/r/2208/#comment8795 > Add back the try/catch protection. This was necessary in some cases (I can't remember why off the top of my head though). /src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java < https://reviews.apache.org/r/2208/#comment8796 > license header is missing. add the std boilerplate found on any other java file in the project. /src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java < https://reviews.apache.org/r/2208/#comment8798 > add a comment to the class describing what this set of tests is for /src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java < https://reviews.apache.org/r/2208/#comment8797 > no tabs, reformat with spaces only. (you'll probably need to cleanup some of your other changes at the same time) /src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java < https://reviews.apache.org/r/2208/#comment8799 > it's fine to ref a jira here, however there should also be some details on what this test is doing. (short summary esp as the test itself is pretty clear) Patrick On 2011-10-05 11:59:30, César Álvarez Núñez wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2208/ ----------------------------------------------------------- (Updated 2011-10-05 11:59:30) Review request for zookeeper. Summary ------- See https://issues.apache.org/jira/browse/ZOOKEEPER-1214 This addresses bug ZOOKEEPER-1214 . https://issues.apache.org/jira/browse/ZOOKEEPER-1214 Diffs ----- /src/java/test/org/apache/zookeeper/test/QuorumUtil.java 1179165 /src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java PRE-CREATION /src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1179166 /src/java/main/org/apache/zookeeper/jmx/MBeanRegistry.java 1169669 Diff: https://reviews.apache.org/r/2208/diff Testing ------- - Thanks, César
        Hide
        César Álvarez Núñez added a comment -

        Patch updated due to review feedback.

        Show
        César Álvarez Núñez added a comment - Patch updated due to review feedback.
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-12-14 23:36:06, Patrick Hunt wrote:

        > /src/java/main/org/apache/zookeeper/jmx/MBeanRegistry.java, lines 126-127

        > <https://reviews.apache.org/r/2208/diff/1/?file=48123#file48123line126>

        >

        > cleanup this typo (2 returns)

        Done.

        On 2011-12-14 23:36:06, Patrick Hunt wrote:

        > /src/java/main/org/apache/zookeeper/jmx/MBeanRegistry.java, line 130

        > <https://reviews.apache.org/r/2208/diff/1/?file=48123#file48123line130>

        >

        > let's call this

        >

        > getRegisteredBeans()

        >

        Ok.

        On 2011-12-14 23:36:06, Patrick Hunt wrote:

        > /src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java, lines 757-767

        > <https://reviews.apache.org/r/2208/diff/1/?file=48124#file48124line757>

        >

        > Add back the try/catch protection. This was necessary in some cases (I can't remember why off the top of my head though).

        I've enhanced the exception handling during "public void unregister(ZKMBeanInfo bean)" method invocation since JMException was caught and logged two times: 1) private void unregister(String path,ZKMBeanInfo bean) throws JMException, 2) public void unregister(ZKMBeanInfo bean).

        I've also added a "catch(Throwable t)" to "public void unregister(ZKMBeanInfo bean)" method that will generate an error log instead of a warn.

        On 2011-12-14 23:36:06, Patrick Hunt wrote:

        > /src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java, line 1

        > <https://reviews.apache.org/r/2208/diff/1/?file=48126#file48126line1>

        >

        > license header is missing. add the std boilerplate found on any other java file in the project.

        As you can see I'm a newbie contributor

        On 2011-12-14 23:36:06, Patrick Hunt wrote:

        > /src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java, line 15

        > <https://reviews.apache.org/r/2208/diff/1/?file=48126#file48126line15>

        >

        > add a comment to the class describing what this set of tests is for

        Done.

        On 2011-12-14 23:36:06, Patrick Hunt wrote:

        > /src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java, line 16

        > <https://reviews.apache.org/r/2208/diff/1/?file=48126#file48126line16>

        >

        > no tabs, reformat with spaces only. (you'll probably need to cleanup some of your other changes at the same time)

        Done. It would be nice that the eclipse ant task will create the .setting files to set all formatting rules and useful save actions (like apply formatting). I'll investigate it and upload as a new jira if success.

        On 2011-12-14 23:36:06, Patrick Hunt wrote:

        > /src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java, line 19

        > <https://reviews.apache.org/r/2208/diff/1/?file=48126#file48126line19>

        >

        > it's fine to ref a jira here, however there should also be some details on what this test is doing. (short summary esp as the test itself is pretty clear)

        Done.

        • César

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

        On 2011-10-05 11:59:30, César Álvarez Núñez wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2208/

        -----------------------------------------------------------

        (Updated 2011-10-05 11:59:30)

        Review request for zookeeper.

        Summary

        -------

        See https://issues.apache.org/jira/browse/ZOOKEEPER-1214

        This addresses bug ZOOKEEPER-1214.

        https://issues.apache.org/jira/browse/ZOOKEEPER-1214

        Diffs

        -----

        /src/java/test/org/apache/zookeeper/test/QuorumUtil.java 1179165

        /src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java PRE-CREATION

        /src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1179166

        /src/java/main/org/apache/zookeeper/jmx/MBeanRegistry.java 1169669

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

        Testing

        -------

        -

        Thanks,

        César

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-12-14 23:36:06, Patrick Hunt wrote: > /src/java/main/org/apache/zookeeper/jmx/MBeanRegistry.java, lines 126-127 > < https://reviews.apache.org/r/2208/diff/1/?file=48123#file48123line126 > > > cleanup this typo (2 returns) Done. On 2011-12-14 23:36:06, Patrick Hunt wrote: > /src/java/main/org/apache/zookeeper/jmx/MBeanRegistry.java, line 130 > < https://reviews.apache.org/r/2208/diff/1/?file=48123#file48123line130 > > > let's call this > > getRegisteredBeans() > Ok. On 2011-12-14 23:36:06, Patrick Hunt wrote: > /src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java, lines 757-767 > < https://reviews.apache.org/r/2208/diff/1/?file=48124#file48124line757 > > > Add back the try/catch protection. This was necessary in some cases (I can't remember why off the top of my head though). I've enhanced the exception handling during "public void unregister(ZKMBeanInfo bean)" method invocation since JMException was caught and logged two times: 1) private void unregister(String path,ZKMBeanInfo bean) throws JMException, 2) public void unregister(ZKMBeanInfo bean). I've also added a "catch(Throwable t)" to "public void unregister(ZKMBeanInfo bean)" method that will generate an error log instead of a warn. On 2011-12-14 23:36:06, Patrick Hunt wrote: > /src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java, line 1 > < https://reviews.apache.org/r/2208/diff/1/?file=48126#file48126line1 > > > license header is missing. add the std boilerplate found on any other java file in the project. As you can see I'm a newbie contributor On 2011-12-14 23:36:06, Patrick Hunt wrote: > /src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java, line 15 > < https://reviews.apache.org/r/2208/diff/1/?file=48126#file48126line15 > > > add a comment to the class describing what this set of tests is for Done. On 2011-12-14 23:36:06, Patrick Hunt wrote: > /src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java, line 16 > < https://reviews.apache.org/r/2208/diff/1/?file=48126#file48126line16 > > > no tabs, reformat with spaces only. (you'll probably need to cleanup some of your other changes at the same time) Done. It would be nice that the eclipse ant task will create the .setting files to set all formatting rules and useful save actions (like apply formatting). I'll investigate it and upload as a new jira if success. On 2011-12-14 23:36:06, Patrick Hunt wrote: > /src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java, line 19 > < https://reviews.apache.org/r/2208/diff/1/?file=48126#file48126line19 > > > it's fine to ref a jira here, however there should also be some details on what this test is doing. (short summary esp as the test itself is pretty clear) Done. César ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2208/#review3914 ----------------------------------------------------------- On 2011-10-05 11:59:30, César Álvarez Núñez wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2208/ ----------------------------------------------------------- (Updated 2011-10-05 11:59:30) Review request for zookeeper. Summary ------- See https://issues.apache.org/jira/browse/ZOOKEEPER-1214 This addresses bug ZOOKEEPER-1214 . https://issues.apache.org/jira/browse/ZOOKEEPER-1214 Diffs ----- /src/java/test/org/apache/zookeeper/test/QuorumUtil.java 1179165 /src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java PRE-CREATION /src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1179166 /src/java/main/org/apache/zookeeper/jmx/MBeanRegistry.java 1169669 Diff: https://reviews.apache.org/r/2208/diff Testing ------- - Thanks, César
        Hide
        Hadoop QA added a comment -

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

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

        +1 tests included. The patch appears to include 6 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/879//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/879//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/879//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/12509566/ZOOKEEPER-1214.patch against trunk revision 1227000. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 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/879//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/879//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/879//console This message is automatically generated.
        Hide
        Patrick Hunt added a comment -

        Can you update the diff on RB, re-submit, and I'll take another look? Thanks.

        Show
        Patrick Hunt added a comment - Can you update the diff on RB, re-submit, and I'll take another look? Thanks.
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        (Updated 2012-01-13 12:10:38.234750)

        Review request for zookeeper.

        Summary
        -------

        See https://issues.apache.org/jira/browse/ZOOKEEPER-1214

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

        Diffs (updated)


        /src/java/main/org/apache/zookeeper/jmx/MBeanRegistry.java 1226094
        /src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1226094
        /src/java/test/org/apache/zookeeper/test/QuorumUtil.java 1226094
        /src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java PRE-CREATION

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

        Testing
        -------

        -

        Thanks,

        César

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2208/ ----------------------------------------------------------- (Updated 2012-01-13 12:10:38.234750) Review request for zookeeper. Summary ------- See https://issues.apache.org/jira/browse/ZOOKEEPER-1214 This addresses bug ZOOKEEPER-1214 . https://issues.apache.org/jira/browse/ZOOKEEPER-1214 Diffs (updated) /src/java/main/org/apache/zookeeper/jmx/MBeanRegistry.java 1226094 /src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1226094 /src/java/test/org/apache/zookeeper/test/QuorumUtil.java 1226094 /src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java PRE-CREATION Diff: https://reviews.apache.org/r/2208/diff Testing ------- - Thanks, César
        Hide
        Michi Mutsuzaki added a comment -

        +1

        Pat, could you take a look at the updated patch? If it looks ok to you, I'll go ahead and check this patch in.

        Thanks!
        --Michi

        Show
        Michi Mutsuzaki added a comment - +1 Pat, could you take a look at the updated patch? If it looks ok to you, I'll go ahead and check this patch in. Thanks! --Michi
        Hide
        Michi Mutsuzaki added a comment -

        Ping!

        --Michi

        Show
        Michi Mutsuzaki added a comment - Ping! --Michi
        Hide
        Michi Mutsuzaki added a comment -

        I saw this error in 3.4.5 with ZOOKEEPER-1214.patch applied.

        2013-02-04 03:42:39,682 503835 [pool-1-thread-6] INFO org.apache.zookeeper.jmx.MBeanRegistry - Unregister MBean [org.apache.ZooKeeperService:name0=ReplicatedServer_id0,name1=replica.0,name2=Follower,name3=Connections,name4=127.0.0.1,name5=0x13ca4fc35c40001]
        2013-02-04 03:42:39,683 503836 [QuorumPeer[myid=0]/127.0.0.1:2901] ERROR org.apache.zookeeper.jmx.MBeanRegistry - Unexpected exception during unregister of [Follower]. It should be reviewed and fixed.
        java.lang.IllegalMonitorStateException
        at java.util.concurrent.locks.ReentrantReadWriteLock$Sync.tryReleaseShared(ReentrantReadWriteLock.java:398)
        at java.util.concurrent.locks.AbstractQueuedSynchronizer.releaseShared(AbstractQueuedSynchronizer.java:1340)
        at java.util.concurrent.locks.ReentrantReadWriteLock$ReadLock.unlock(ReentrantReadWriteLock.java:778)
        at com.sun.jmx.mbeanserver.Repository.retrieve(Repository.java:522)
        at com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.getMBean(DefaultMBeanServerInterceptor.java:1111)
        at com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.exclusiveUnregisterMBean(DefaultMBeanServerInterceptor.java:433)
        at com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.unregisterMBean(DefaultMBeanServerInterceptor.java:421)
        at com.sun.jmx.mbeanserver.JmxMBeanServer.unregisterMBean(JmxMBeanServer.java:540)
        at org.apache.zookeeper.jmx.MBeanRegistry.unregister(MBeanRegistry.java:116)
        at org.apache.zookeeper.jmx.MBeanRegistry.unregister(MBeanRegistry.java:137)
        at org.apache.zookeeper.server.quorum.LearnerZooKeeperServer.unregisterJMX(LearnerZooKeeperServer.java:138)
        at org.apache.zookeeper.server.quorum.Follower.followLeader(Follower.java:100)
        at org.apache.zookeeper.server.quorum.QuorumPeer.run(QuorumPeer.java:745)

        I'm using openjdk-6-jre-6b24-1.11.3-1ubuntu0.12.04.1. Should I use sun-java6-jre instead?

        --Michi

        Show
        Michi Mutsuzaki added a comment - I saw this error in 3.4.5 with ZOOKEEPER-1214 .patch applied. 2013-02-04 03:42:39,682 503835 [pool-1-thread-6] INFO org.apache.zookeeper.jmx.MBeanRegistry - Unregister MBean [org.apache.ZooKeeperService:name0=ReplicatedServer_id0,name1=replica.0,name2=Follower,name3=Connections,name4=127.0.0.1,name5=0x13ca4fc35c40001] 2013-02-04 03:42:39,683 503836 [QuorumPeer [myid=0] /127.0.0.1:2901] ERROR org.apache.zookeeper.jmx.MBeanRegistry - Unexpected exception during unregister of [Follower] . It should be reviewed and fixed. java.lang.IllegalMonitorStateException at java.util.concurrent.locks.ReentrantReadWriteLock$Sync.tryReleaseShared(ReentrantReadWriteLock.java:398) at java.util.concurrent.locks.AbstractQueuedSynchronizer.releaseShared(AbstractQueuedSynchronizer.java:1340) at java.util.concurrent.locks.ReentrantReadWriteLock$ReadLock.unlock(ReentrantReadWriteLock.java:778) at com.sun.jmx.mbeanserver.Repository.retrieve(Repository.java:522) at com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.getMBean(DefaultMBeanServerInterceptor.java:1111) at com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.exclusiveUnregisterMBean(DefaultMBeanServerInterceptor.java:433) at com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.unregisterMBean(DefaultMBeanServerInterceptor.java:421) at com.sun.jmx.mbeanserver.JmxMBeanServer.unregisterMBean(JmxMBeanServer.java:540) at org.apache.zookeeper.jmx.MBeanRegistry.unregister(MBeanRegistry.java:116) at org.apache.zookeeper.jmx.MBeanRegistry.unregister(MBeanRegistry.java:137) at org.apache.zookeeper.server.quorum.LearnerZooKeeperServer.unregisterJMX(LearnerZooKeeperServer.java:138) at org.apache.zookeeper.server.quorum.Follower.followLeader(Follower.java:100) at org.apache.zookeeper.server.quorum.QuorumPeer.run(QuorumPeer.java:745) I'm using openjdk-6-jre-6b24-1.11.3-1ubuntu0.12.04.1. Should I use sun-java6-jre instead? --Michi
        Hide
        Edward Ribeiro added a comment -

        https://issues.apache.org/jira/browse/ZOOKEEPER-1214

        Hi Michi,

        My two cents: this looks like a bug in ReentrantReadWriteLock, the JVM, or even an incorrect use of ReentrantReadWriteLock by JmxMBeanServer. I say so because a quick Google search shows that other projects that use ReentrantReadWriteLock, directly or not, as HBase, Netbeans and Maven have hit this very same exception.

        As ZooKeeper uses ReentrantReadWriteLock indirectly through JmxMBeanServer, a possible solution seems to be to create a lock object and synchronize lines 98 and 117 of JmxMBeanServer as follows:

        JmxMBeanServer.java
        ...
        private final Object LOCK = new Object();
        ...
        // line 98 becomes
          synchronize (LOCK) {
             mBeanServer.registerMBean(bean, oname);            
          }
        // line 117 becomes
          synchronize (LOCK) {
             mBeanServer.unregisterMBean(makeObjectName(path, bean));
          }
        

        As this is a issue addressed to Cesar I'll not submit a patch, but I may provide you a patch by email to see if this solution works.

        Cheers,
        Edward

        Show
        Edward Ribeiro added a comment - https://issues.apache.org/jira/browse/ZOOKEEPER-1214 Hi Michi, My two cents: this looks like a bug in ReentrantReadWriteLock, the JVM, or even an incorrect use of ReentrantReadWriteLock by JmxMBeanServer. I say so because a quick Google search shows that other projects that use ReentrantReadWriteLock, directly or not , as HBase, Netbeans and Maven have hit this very same exception. As ZooKeeper uses ReentrantReadWriteLock indirectly through JmxMBeanServer, a possible solution seems to be to create a lock object and synchronize lines 98 and 117 of JmxMBeanServer as follows: JmxMBeanServer.java ... private final Object LOCK = new Object (); ... // line 98 becomes synchronize (LOCK) { mBeanServer.registerMBean(bean, oname); } // line 117 becomes synchronize (LOCK) { mBeanServer.unregisterMBean(makeObjectName(path, bean)); } As this is a issue addressed to Cesar I'll not submit a patch, but I may provide you a patch by email to see if this solution works. Cheers, Edward
        Hide
        Edward Ribeiro added a comment -

        Ummm ... as this issue last patch is +1 year old – and I made a silly mistake during my previous explanation (sorry, it's synchronized keyword instead of synchronize) – then I decided to submit a new version of Cesar's patch that applies against the trunk and implements the possible solution I've just proposed. It still Cesar's patch tough. See if it helps.

        Show
        Edward Ribeiro added a comment - Ummm ... as this issue last patch is +1 year old – and I made a silly mistake during my previous explanation (sorry, it's synchronized keyword instead of synchronize ) – then I decided to submit a new version of Cesar's patch that applies against the trunk and implements the possible solution I've just proposed. It still Cesar's patch tough. See if it helps.
        Hide
        Michi Mutsuzaki added a comment -

        Thank you for the patch, Edward.

        This patch looks good to me. I'd like to check it in if nobody is against it.

        --Michi

        Show
        Michi Mutsuzaki added a comment - Thank you for the patch, Edward. This patch looks good to me. I'd like to check it in if nobody is against it. --Michi
        Hide
        Edward Ribeiro added a comment -

        Thanks for the feedback Michi, it's highly appreciated. I am sending a new version with an one line change wrt the previous one.

        Show
        Edward Ribeiro added a comment - Thanks for the feedback Michi, it's highly appreciated. I am sending a new version with an one line change wrt the previous one.
        Hide
        Michi Mutsuzaki added a comment -

        Sorry I forgot about this patch. I'll rebase it and check it in.

        Show
        Michi Mutsuzaki added a comment - Sorry I forgot about this patch. I'll rebase it and check it in.
        Hide
        Hadoop QA added a comment -

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

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

        +1 tests included. The patch appears to include 5 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/2070//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2070//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2070//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/12642729/ZOOKEEPER-1214.patch against trunk revision 1591175. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 5 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/2070//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2070//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2070//console This message is automatically generated.
        Hide
        Hadoop QA added a comment -

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

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

        +1 tests included. The patch appears to include 5 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/2101//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2101//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2101//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/12642729/ZOOKEEPER-1214.patch against trunk revision 1595559. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 5 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/2101//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2101//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2101//console This message is automatically generated.
        Show
        Michi Mutsuzaki added a comment - trunk: http://svn.apache.org/viewvc?view=revision&revision=1595561
        Hide
        Hudson added a comment -

        FAILURE: Integrated in ZooKeeper-trunk #2311 (See https://builds.apache.org/job/ZooKeeper-trunk/2311/)
        ZOOKEEPER-1214. QuorumPeer should unregister only its previsously registered MBeans instead of use MBeanRegistry.unregisterAll() method. (César Álvarez Núñez via michim) (michim: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1595561)

        • /zookeeper/trunk/CHANGES.txt
        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/jmx/MBeanRegistry.java
        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java
        • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/QuorumUtil.java
        • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java
        Show
        Hudson added a comment - FAILURE: Integrated in ZooKeeper-trunk #2311 (See https://builds.apache.org/job/ZooKeeper-trunk/2311/ ) ZOOKEEPER-1214 . QuorumPeer should unregister only its previsously registered MBeans instead of use MBeanRegistry.unregisterAll() method. (César Álvarez Núñez via michim) (michim: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1595561 ) /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/src/java/main/org/apache/zookeeper/jmx/MBeanRegistry.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/QuorumUtil.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java

          People

          • Assignee:
            César Álvarez Núñez
            Reporter:
            César Álvarez Núñez
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development