Details

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

      Description

      Server has many critical threads running and co-ordinating each other like RequestProcessor chains et. When going through each threads, most of them having the similar structure like:

      public void run() {
              try {
                    while(running)
                         // processing logic
                    }
              } catch (InterruptedException e) {
                  LOG.error("Unexpected interruption", e);
              } catch (Exception e) {
                  LOG.error("Unexpected exception", e);
              }
              LOG.info("...exited loop!");
      }
      

      From the design I could see, there could be a chance of silently leaving the thread by swallowing the exception. If this happens in the production, the server would get hanged forever and would not be able to deliver its role. Now its hard for the management tool to detect this.

      The idea of this JIRA is to discuss and imprv.

      Reference: Community discussion thread

      1. ZOOKEEPER-1907.patch
        19 kB
        Rakesh R
      2. ZOOKEEPER-1907.patch
        20 kB
        Rakesh R
      3. ZOOKEEPER-1907.patch
        19 kB
        Rakesh R
      4. ZOOKEEPER-1907.patch
        23 kB
        Rakesh R
      5. ZOOKEEPER-1907.patch
        24 kB
        Rakesh R
      6. ZOOKEEPER-1907.patch
        24 kB
        Rakesh R
      7. ZOOKEEPER-1907.patch
        28 kB
        Rakesh R
      8. ZOOKEEPER-1907.patch
        35 kB
        Rakesh R
      9. ZOOKEEPER-1907.patch
        38 kB
        Rakesh R
      10. ZOOKEEPER-1907.patch
        39 kB
        Rakesh R
      11. ZOOKEEPER-1907.patch
        33 kB
        Rakesh R
      12. ZOOKEEPER-1907.patch
        35 kB
        Rakesh R
      13. ZOOKEEPER-1907.patch
        34 kB
        Rakesh R

        Issue Links

          Activity

          Hide
          Rakesh R added a comment -

          I'm attaching an initial proposal where I'm addressing critical threads like:

          • CommitProcessor
          • FollowerRequestProcessor
          • ObserverRequestProcessor
          • PrepRequestProcessor
          • ReadOnlyRequestProcessor
          • SessionTrackerImpl
          • SyncRequestProcessor

          Please review the approach and the patch. Thanks in advance.

          Show
          Rakesh R added a comment - I'm attaching an initial proposal where I'm addressing critical threads like: CommitProcessor FollowerRequestProcessor ObserverRequestProcessor PrepRequestProcessor ReadOnlyRequestProcessor SessionTrackerImpl SyncRequestProcessor Please review the approach and the patch. Thanks in advance.
          Hide
          Hadoop QA added a comment -

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

          +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/2008//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/12637862/ZOOKEEPER-1907.patch against trunk revision 1583083. +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/2008//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/12637862/ZOOKEEPER-1907.patch
          against trunk revision 1583083.

          +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/2009//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/12637862/ZOOKEEPER-1907.patch against trunk revision 1583083. +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/2009//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/12638002/ZOOKEEPER-1907.patch
          against trunk revision 1583513.

          +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/2016//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/12638002/ZOOKEEPER-1907.patch against trunk revision 1583513. +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/2016//console This message is automatically generated.
          Hide
          Michi Mutsuzaki added a comment -

          Thank you for the patch Rakesh. Could you put this on the reviewboard?

          Show
          Michi Mutsuzaki added a comment - Thank you for the patch Rakesh. Could you put this on the reviewboard?
          Hide
          Raul Gutierrez Segales added a comment -

          Commented on the RB.

          Show
          Raul Gutierrez Segales added a comment - Commented on the RB.
          Hide
          Raul Gutierrez Segales added a comment -

          (And I forgot to add, this is awesome - thanks Rakesh R).

          Show
          Raul Gutierrez Segales added a comment - (And I forgot to add, this is awesome - thanks Rakesh R ).
          Hide
          Rakesh R added a comment -

          Forgot to mention review ticket link : https://reviews.apache.org/r/20071/

          Show
          Rakesh R added a comment - Forgot to mention review ticket link : https://reviews.apache.org/r/20071/
          Hide
          Raul Gutierrez Segales added a comment -

          Rakesh R: were you planning on updating that RB with (some of) the comments I made? (I see you posted the link to the RB, but didn't see an updated diff).

          Show
          Raul Gutierrez Segales added a comment - Rakesh R : were you planning on updating that RB with (some of) the comments I made? (I see you posted the link to the RB, but didn't see an updated diff).
          Hide
          Rakesh R added a comment -

          Raul Gutierrez Segales, before preparing the patch I would like to discuss about one of your comments and make it clear. I have replied sometime back in RB, could you please have a look at it. Thanks!

          Show
          Rakesh R added a comment - Raul Gutierrez Segales , before preparing the patch I would like to discuss about one of your comments and make it clear. I have replied sometime back in RB, could you please have a look at it. Thanks!
          Hide
          Hadoop QA added a comment -

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

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

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

          Rakesh R and Raul Gutierrez Segales any progress on this? I can't tell from the most recent comment(s) if this is ready or still updates are planned? Please "cancel" the patch if it's not ready for submission. Thanks.

          Show
          Patrick Hunt added a comment - Rakesh R and Raul Gutierrez Segales any progress on this? I can't tell from the most recent comment(s) if this is ready or still updates are planned? Please "cancel" the patch if it's not ready for submission. Thanks.
          Hide
          Rakesh R added a comment -

          Patrick Hunt there is one open comment which needs to be concluded. Actually this is marked for 3.6.0. IMO if reach to an agreement will include this in 3.5.0 ?

          Raul Gutierrez Segales sometime back I've replied to your comment in RB. I'd like to know your thoughts. Thanks.

          Show
          Rakesh R added a comment - Patrick Hunt there is one open comment which needs to be concluded. Actually this is marked for 3.6.0. IMO if reach to an agreement will include this in 3.5.0 ? Raul Gutierrez Segales sometime back I've replied to your comment in RB. I'd like to know your thoughts. Thanks.
          Hide
          Patrick Hunt added a comment -

          I think if everyone is ok with the change we could include it in 3.5.0. What do you think?

          Show
          Patrick Hunt added a comment - I think if everyone is ok with the change we could include it in 3.5.0. What do you think?
          Hide
          Rakesh R added a comment -

          That would be fine.

          Show
          Rakesh R added a comment - That would be fine.
          Hide
          Raul Gutierrez Segales added a comment -

          I'd very much like to have this for 3.5.0. Let me quickly check the comments Rakesh R is referring to.

          Show
          Raul Gutierrez Segales added a comment - I'd very much like to have this for 3.5.0. Let me quickly check the comments Rakesh R is referring to.
          Hide
          Raul Gutierrez Segales added a comment -

          +1 && shipit on the reviewboard — thanks Rakesh R and Patrick Hunt!

          Show
          Raul Gutierrez Segales added a comment - +1 && shipit on the reviewboard — thanks Rakesh R and Patrick Hunt !
          Hide
          Rakesh R added a comment -

          Thanks Raul Gutierrez Segales for the reviews. I've rebased the RB patch in trunk and attached the same.

          Show
          Rakesh R added a comment - Thanks Raul Gutierrez Segales for the reviews. I've rebased the RB patch in trunk and attached the same.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 12 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 appears to introduce 87 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/2204//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2204//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2204//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/12656748/ZOOKEEPER-1907.patch against trunk revision 1611846. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 12 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 appears to introduce 87 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/2204//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2204//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2204//console This message is automatically generated.
          Hide
          Hongchao Deng added a comment -

          Rakesh R
          I just downloaded your patch and git warns that the following files end with "CR".

          ./src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java
          ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java
          ./src/java/main/org/apache/zookeeper/server/SessionTrackerImpl.java
          
          Show
          Hongchao Deng added a comment - Rakesh R I just downloaded your patch and git warns that the following files end with "CR". ./src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java ./src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java ./src/java/main/org/apache/zookeeper/server/SessionTrackerImpl.java
          Hide
          Rakesh R added a comment -

          Hi Hongchao Deng, I've uploaded a new patch, please try this. Thanks!

          Show
          Rakesh R added a comment - Hi Hongchao Deng , I've uploaded a new patch, please try this. Thanks!
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 12 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 appears to introduce 1 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/2213//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2213//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2213//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/12657067/ZOOKEEPER-1907.patch against trunk revision 1612458. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 12 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 appears to introduce 1 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/2213//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2213//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2213//console This message is automatically generated.
          Hide
          Hongchao Deng added a comment -

          Rakesh R
          My jenkins job reported that

          Test org.apache.zookeeper.server.quorum.CommitProcessorTest FAILED (crashed)
          
          Show
          Hongchao Deng added a comment - Rakesh R My jenkins job reported that Test org.apache.zookeeper.server.quorum.CommitProcessorTest FAILED (crashed)
          Hide
          Hongchao Deng added a comment -

          SessionTrackerImpl.java still contains new line

          Show
          Hongchao Deng added a comment - SessionTrackerImpl.java still contains new line
          Hide
          Patrick Hunt added a comment -

          Rakesh R any changes per Hongchao's comments? Or are we ok to move forward with this?

          Show
          Patrick Hunt added a comment - Rakesh R any changes per Hongchao's comments? Or are we ok to move forward with this?
          Hide
          Rakesh R added a comment -

          Hongchao Deng Patrick Hunt sorry for the delay I was busy with other schedules.

          SessionTrackerImpl.java still contains new line

          I have checked the latest patch by exec the following command in my unix machine to verify the newline characters. Its not showing any problem for me.

          #!/usr/bin/env bash
          
          if awk  '/\r$/{exit 0;} 1{exit 1;}' ZOOKEEPER-1907.patch
          then
            echo "is DOS"
          fi
          
          if [[ "$(head -1 ZOOKEEPER-1907.patch)" == *$'\r' ]]; then echo DOS; fi
          

          Test org.apache.zookeeper.server.quorum.CommitProcessorTest FAILED (crashed)

          Good catch. I hope the test case is failing due to the following reason. I will fix soon and update a patch. Please ping me any other causes. Thanks!

          2014-07-24 10:56:37,543 [myid:] - ERROR [ZKDeathWatcherThread:ZooKeeperCriticalThread@47] - Severe unrecoverable error, from thread : ZKDeathWatcherThread
          java.lang.NullPointerException
          	at org.apache.zookeeper.server.ZooKeeperServer$ZKDeathWatcher.run(ZooKeeperServer.java:1084)
          
          Show
          Rakesh R added a comment - Hongchao Deng Patrick Hunt sorry for the delay I was busy with other schedules. SessionTrackerImpl.java still contains new line I have checked the latest patch by exec the following command in my unix machine to verify the newline characters. Its not showing any problem for me. #!/usr/bin/env bash if awk '/\r$/{exit 0;} 1{exit 1;}' ZOOKEEPER-1907.patch then echo "is DOS" fi if [[ "$(head -1 ZOOKEEPER-1907.patch)" == *$'\r' ]]; then echo DOS; fi Test org.apache.zookeeper.server.quorum.CommitProcessorTest FAILED (crashed) Good catch. I hope the test case is failing due to the following reason. I will fix soon and update a patch. Please ping me any other causes. Thanks! 2014-07-24 10:56:37,543 [myid:] - ERROR [ZKDeathWatcherThread:ZooKeeperCriticalThread@47] - Severe unrecoverable error, from thread : ZKDeathWatcherThread java.lang.NullPointerException at org.apache.zookeeper.server.ZooKeeperServer$ZKDeathWatcher.run(ZooKeeperServer.java:1084)
          Hide
          Hadoop QA added a comment -

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

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

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

          Hongchao Deng Attached latest patch where I've corrected the test case. Could you have a look at this. Thanks!

          Show
          Rakesh R added a comment - Hongchao Deng Attached latest patch where I've corrected the test case. Could you have a look at this. Thanks!
          Hide
          Hongchao Deng added a comment -

          hi Rakesh R.
          The patch should work. However, I didn't like the idea of while-loop polling each thread's live status.
          Could it use thread-join like notification based mechanism?

          Another thing I understand from the code (if correctly) is when a thread died, the entire ZK process is shutdown. If so, what is the difference if just letting the exception go all the way up and shut it down? I am wondering that the original purpose was to try restarting or so.

          I am not familiar with this code. So any correction on my statement is welcome.

          Show
          Hongchao Deng added a comment - hi Rakesh R . The patch should work. However, I didn't like the idea of while-loop polling each thread's live status. Could it use thread-join like notification based mechanism? Another thing I understand from the code (if correctly) is when a thread died, the entire ZK process is shutdown. If so, what is the difference if just letting the exception go all the way up and shut it down? I am wondering that the original purpose was to try restarting or so. I am not familiar with this code. So any correction on my statement is welcome.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12666793/ZOOKEEPER-1907.zip
          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 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2313//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/12666793/ZOOKEEPER-1907.zip 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 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2313//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/12666828/ZOOKEEPER-1907.patch
          against trunk revision 1621313.

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

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

          +1 javadoc. The javadoc tool did not generate any warning messages.

          -1 javac. The patch appears to cause tar ant target to fail.

          -1 findbugs. The patch appears to cause Findbugs (version 2.0.3) to fail.

          +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/2314//testReport/
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2314//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/12666828/ZOOKEEPER-1907.patch against trunk revision 1621313. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 18 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. -1 javac. The patch appears to cause tar ant target to fail. -1 findbugs. The patch appears to cause Findbugs (version 2.0.3) to fail. +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/2314//testReport/ Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2314//console This message is automatically generated.
          Hide
          Rakesh R added a comment -

          Thanks Hongchao Deng for the comments

          I didn't like the idea of while-loop polling each thread's live status.

          I've tried different approach by passing listeners to the critical threads and handing the exception. Please have a look at the patch(yet to add any testcases)

          Another thing I understand from the code (if correctly) is when a thread died, the entire ZK process is shutdown. If so, what is the difference if just letting the exception go all the way up and shut it down? I am wondering that the original purpose was to try restarting or so.

          I think restarting the critical resources will make it more complex. Like I mentioned at the beginning there are many critical threads and I'm afraid of inconsistencies. Simple way of handling is, shutdown and leave the things to administrator/monitoring tool, which can restart back after rectifying the cause(For OOME, any functional errors et.).

          Show
          Rakesh R added a comment - Thanks Hongchao Deng for the comments I didn't like the idea of while-loop polling each thread's live status. I've tried different approach by passing listeners to the critical threads and handing the exception. Please have a look at the patch(yet to add any testcases) Another thing I understand from the code (if correctly) is when a thread died, the entire ZK process is shutdown. If so, what is the difference if just letting the exception go all the way up and shut it down? I am wondering that the original purpose was to try restarting or so. I think restarting the critical resources will make it more complex. Like I mentioned at the beginning there are many critical threads and I'm afraid of inconsistencies. Simple way of handling is, shutdown and leave the things to administrator/monitoring tool, which can restart back after rectifying the cause(For OOME, any functional errors et.).
          Hide
          Rakesh R added a comment -

          Hongchao Deng, it would be great to see your feedback on my comments. Thanks!

          Show
          Rakesh R added a comment - Hongchao Deng , it would be great to see your feedback on my comments. Thanks!
          Hide
          Hadoop QA added a comment -

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

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

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

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

          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2474//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/12666828/ZOOKEEPER-1907.patch against trunk revision 1646992. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 18 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2474//console This message is automatically generated.
          Hide
          Hongchao Deng added a comment -

          Hi Rakesh R. Thanks for notifying.

          I wondered if throwing the exception up could be a simple alternative? In high level, the problem seems to be that we want to get other threads die if one thread die. Because if one thread goes away, the ZK server hangs. Why not just let the exception be thrown up and kill the program? I am not sure about the intention behind those work of listeners.

          Show
          Hongchao Deng added a comment - Hi Rakesh R . Thanks for notifying. I wondered if throwing the exception up could be a simple alternative? In high level, the problem seems to be that we want to get other threads die if one thread die. Because if one thread goes away, the ZK server hangs. Why not just let the exception be thrown up and kill the program? I am not sure about the intention behind those work of listeners.
          Hide
          Rakesh R added a comment -

          Why not just let the exception be thrown up and kill the program?

          Do you mean System.exit() ?. If yes, it will affect the embedded mode of ZK deployment. There is already a jira ZOOKEEPER-575 to replace System.exit calls.
          The idea of listener is to gracefully shutdown the server if one of the critical thread dies.

          Show
          Rakesh R added a comment - Why not just let the exception be thrown up and kill the program? Do you mean System.exit() ?. If yes, it will affect the embedded mode of ZK deployment. There is already a jira ZOOKEEPER-575 to replace System.exit calls. The idea of listener is to gracefully shutdown the server if one of the critical thread dies.
          Hide
          Hongchao Deng added a comment -

          Great. Thanks for explanation. Thumbs up to the graceful shutdown.

          I skim through the patch and have no problem with it. Would you mind to update the patch to the latest trunk again and put it on ReviewBoard? I would be glad to look at and discuss the coding

          Show
          Hongchao Deng added a comment - Great. Thanks for explanation. Thumbs up to the graceful shutdown. I skim through the patch and have no problem with it. Would you mind to update the patch to the latest trunk again and put it on ReviewBoard? I would be glad to look at and discuss the coding
          Hide
          Rakesh R added a comment -

          Thanks for the reply. I will rebase the patch in latest trunk and upload it.

          Show
          Rakesh R added a comment - Thanks for the reply. I will rebase the patch in latest trunk and upload it.
          Hide
          Rakesh R added a comment -

          Updated review ticket https://reviews.apache.org/r/20071 with new patch. Appreciate any comments. Thanks!

          Show
          Rakesh R added a comment - Updated review ticket https://reviews.apache.org/r/20071 with new patch. Appreciate any comments. Thanks!
          Hide
          Rakesh R added a comment -

          Attaching latest patch(same is uploaded to review board).

          Show
          Rakesh R added a comment - Attaching latest patch(same is uploaded to review board).
          Hide
          Hadoop QA added a comment -

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

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

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

          Rakesh R.

          Added some comments. I have two concerns and let's discuss it in the JIRA:

          1. The handleException() in ZK critical Thread seems to be duplicate to the added "notify and shut it down" function. How can we make this part cleaner?

          2. What about multiple calls to shutdown()?

          Show
          Hongchao Deng added a comment - Rakesh R . Added some comments. I have two concerns and let's discuss it in the JIRA: 1. The handleException() in ZK critical Thread seems to be duplicate to the added "notify and shut it down" function. How can we make this part cleaner? 2. What about multiple calls to shutdown()?
          Hide
          Rakesh R added a comment -

          Thanks Hongchao Deng for the comments.

          1. handleException() in ZK critical Thread seems to be duplicate to the added "notify and shut it down" function. How can we make this part cleaner?

          instead of notifying, can call #handleException(thName, exp). I will also remove the duplicate error log messages in the #run() method. Does this sound good to you?

          2.What about multiple calls to shutdown()?

          I think, this.running flag present in ZooKeeperServer will help to avoid duplicate calls. Before calling zks#shutdown() will do a check zks#isRunning(), let me try this out.

          Show
          Rakesh R added a comment - Thanks Hongchao Deng for the comments. 1. handleException() in ZK critical Thread seems to be duplicate to the added "notify and shut it down" function. How can we make this part cleaner? instead of notifying, can call #handleException(thName, exp). I will also remove the duplicate error log messages in the #run() method. Does this sound good to you? 2.What about multiple calls to shutdown()? I think, this.running flag present in ZooKeeperServer will help to avoid duplicate calls. Before calling zks#shutdown() will do a check zks#isRunning(), let me try this out.
          Hide
          Rakesh R added a comment -

          Before calling zks#shutdown() will do a check zks#isRunning(), let me try this out.

          ZooKeeperServer is setting the flag this.running=true at the end of the #startup() . Now, there could be a corner case like, ZooKeeperServerListener notification would be skipped if there any fatal notification comes during the server starting up phase.

          An alternative approach I'm thinking is, add a notified flag in ZooKeeperServerListenerImpl. Set notified=false at the beginning of ZooKeeperServer #startup() and just before invoking ZooKeeperServer#shutdown() set notified=true. Also, add a pre-condition if(notified) to skip the ZooKeeperServer#shutdown() if already notified.

          Show
          Rakesh R added a comment - Before calling zks#shutdown() will do a check zks#isRunning(), let me try this out. ZooKeeperServer is setting the flag this.running=true at the end of the #startup() . Now, there could be a corner case like, ZooKeeperServerListener notification would be skipped if there any fatal notification comes during the server starting up phase. An alternative approach I'm thinking is, add a notified flag in ZooKeeperServerListenerImpl. Set notified=false at the beginning of ZooKeeperServer #startup() and just before invoking ZooKeeperServer#shutdown() set notified=true . Also, add a pre-condition if(notified) to skip the ZooKeeperServer#shutdown() if already notified.
          Hide
          Hongchao Deng added a comment -

          My thought: startup() and shutdown() should be sequential. If not, this is a good chance to fix it.

          Show
          Hongchao Deng added a comment - My thought: startup() and shutdown() should be sequential. If not, this is a good chance to fix it.
          Hide
          Rakesh R added a comment -

          Thanks Hongchao Deng. Attached patch addressing the comments.

          Show
          Rakesh R added a comment - Thanks Hongchao Deng . Attached patch addressing the comments.
          Hide
          Hadoop QA added a comment -

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

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

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

          +1 I'll wait for Hongchao's +1 before checking this in. Thanks Rakesh!

          Show
          Michi Mutsuzaki added a comment - +1 I'll wait for Hongchao's +1 before checking this in. Thanks Rakesh!
          Hide
          Hongchao Deng added a comment -

          Hi Rakesh R.

          Added some light comments in RB. I will wait for your next patch fixing the shutdown(), running() stuff and do a further review. Thanks!

          Show
          Hongchao Deng added a comment - Hi Rakesh R . Added some light comments in RB. I will wait for your next patch fixing the shutdown(), running() stuff and do a further review. Thanks!
          Hide
          Rakesh R added a comment -

          Attaching the latest patch to the jira (the same patch is uploaded to RB as well). Please have a look.

          Show
          Rakesh R added a comment - Attaching the latest patch to the jira (the same patch is uploaded to RB as well). Please have a look.
          Hide
          Hadoop QA added a comment -

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

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

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

          The latest patch looks really good to me, +1!

          I have a concern which is not related to the patch:
          In ZooKeeperServer#submitRequest():

               if (firstProcessor == null) {
                      synchronized (this) {
                          try {
                              while (!running) {
                                  wait(1000);
                              }
                          } catch (InterruptedException e) {
                              LOG.warn("Unexpected interruption", e);
                          }
                          if (firstProcessor == null) {
                              throw new RuntimeException("Not started");
                          }
                      }
                  }
          

          I think the purpose of this code is to wait on initial setup. I can see there's a race that after shutdown() "running" is set to false. And the while loop will go forever. Moreover, submitRequest() doesn't handle the case when server is shut down. It seems fine because it's shut down.

          I really hope the first "running" race could be fixed in this issue. A way I can think of would be to change running from bool to int (or enum: STARTING, RUNNING, SHUTDOWN).

          Show
          Hongchao Deng added a comment - The latest patch looks really good to me, +1! I have a concern which is not related to the patch: In ZooKeeperServer#submitRequest(): if (firstProcessor == null ) { synchronized ( this ) { try { while (!running) { wait(1000); } } catch (InterruptedException e) { LOG.warn( "Unexpected interruption" , e); } if (firstProcessor == null ) { throw new RuntimeException( "Not started" ); } } } I think the purpose of this code is to wait on initial setup. I can see there's a race that after shutdown() "running" is set to false. And the while loop will go forever. Moreover, submitRequest() doesn't handle the case when server is shut down. It seems fine because it's shut down. I really hope the first "running" race could be fixed in this issue. A way I can think of would be to change running from bool to int (or enum: STARTING, RUNNING, SHUTDOWN).
          Hide
          Rakesh R added a comment -

          Hongchao Deng Thats really good catch. This case can occur and need to handle it.

          Show
          Rakesh R added a comment - Hongchao Deng Thats really good catch. This case can occur and need to handle it.
          Hide
          Hadoop QA added a comment -

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

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

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

          It seems test case failure is unrelated to this patch.

          I really hope the first "running" race could be fixed in this issue. A way I can think of would be to change running from bool to int.

          Hongchao Deng I've uploaded new patch which uses enum State to address this case. Please have a look at the latest patch. Thanks!

          Show
          Rakesh R added a comment - It seems test case failure is unrelated to this patch. I really hope the first "running" race could be fixed in this issue. A way I can think of would be to change running from bool to int. Hongchao Deng I've uploaded new patch which uses enum State to address this case. Please have a look at the latest patch. Thanks!
          Hide
          Hongchao Deng added a comment -

          Thanks for notifying.
          I realized RB not showing the latest. Can you update it?
          I will review it tomorrow (Monday for me).

          Show
          Hongchao Deng added a comment - Thanks for notifying. I realized RB not showing the latest. Can you update it? I will review it tomorrow (Monday for me).
          Hide
          Hongchao Deng added a comment -

          Hi Rakesh R, could you update the RB to the latest patch?

          Show
          Hongchao Deng added a comment - Hi Rakesh R , could you update the RB to the latest patch?
          Hide
          Rakesh R added a comment -

          I've updated the RB, could you have a look at it.

          Show
          Rakesh R added a comment - I've updated the RB, could you have a look at it.
          Hide
          Rakesh R added a comment -

          Attached new patch addressing the comments given by Hongchao Deng in the review board.

          Show
          Rakesh R added a comment - Attached new patch addressing the comments given by Hongchao Deng in the review board.
          Hide
          Hadoop QA added a comment -

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

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

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

          +1
          Thanks for your work, Rakesh R.

          Show
          Hongchao Deng added a comment - +1 Thanks for your work, Rakesh R .
          Hide
          Rakesh R added a comment -

          Thanks a lot Hongchao Deng for shaping the solution and reviews.

          Note: Test case failure is unrelated to this patch.

          Show
          Rakesh R added a comment - Thanks a lot Hongchao Deng for shaping the solution and reviews. Note: Test case failure is unrelated to this patch.
          Hide
          Rakesh R added a comment -

          Michi Mutsuzaki could you have a look at the latest patch when you get some time. Thanks!

          Show
          Rakesh R added a comment - Michi Mutsuzaki could you have a look at the latest patch when you get some time. Thanks!
          Hide
          Michi Mutsuzaki added a comment -

          +1 Thanks Rakesh!

          Show
          Michi Mutsuzaki added a comment - +1 Thanks Rakesh!
          Show
          Michi Mutsuzaki added a comment - trunk: http://svn.apache.org/viewvc?view=revision&revision=1665089 branch-3.5: http://svn.apache.org/viewvc?view=revision&revision=1665090
          Hide
          Rakesh R added a comment -

          Thanks Hongchao Deng,Raul Gutierrez Segales,Michi Mutsuzaki for the reviews and committing the changes.

          Show
          Rakesh R added a comment - Thanks Hongchao Deng , Raul Gutierrez Segales , Michi Mutsuzaki for the reviews and committing the changes.
          Hide
          Hudson added a comment -

          FAILURE: Integrated in ZooKeeper-trunk #2619 (See https://builds.apache.org/job/ZooKeeper-trunk/2619/)
          ZOOKEEPER-1907 Improve Thread handling (Rakesh R via michim) (michim: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1665089)

          • /zookeeper/trunk/CHANGES.txt
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ExitCode.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/SessionTrackerImpl.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/SyncRequestProcessor.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperCriticalThread.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperServerListener.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/FollowerRequestProcessor.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/FollowerZooKeeperServer.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/LeaderSessionTracker.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/LeaderZooKeeperServer.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/LearnerSessionTracker.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/LearnerZooKeeperServer.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/LocalSessionTracker.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/ObserverRequestProcessor.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/ObserverZooKeeperServer.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/ReadOnlyRequestProcessor.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/ReadOnlyZooKeeperServer.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/UpgradeableSessionTracker.java
          • /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/ZooKeeperThreadTest.java
          • /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java
          • /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java
          • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/ClientBase.java
          • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/SessionTrackerCheckTest.java
          Show
          Hudson added a comment - FAILURE: Integrated in ZooKeeper-trunk #2619 (See https://builds.apache.org/job/ZooKeeper-trunk/2619/ ) ZOOKEEPER-1907 Improve Thread handling (Rakesh R via michim) (michim: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1665089 ) /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ExitCode.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/SessionTrackerImpl.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/SyncRequestProcessor.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperCriticalThread.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperServerListener.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/FollowerRequestProcessor.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/FollowerZooKeeperServer.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/LeaderSessionTracker.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/LeaderZooKeeperServer.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/LearnerSessionTracker.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/LearnerZooKeeperServer.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/LocalSessionTracker.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/ObserverRequestProcessor.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/ObserverZooKeeperServer.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/ReadOnlyRequestProcessor.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/ReadOnlyZooKeeperServer.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/UpgradeableSessionTracker.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/ZooKeeperThreadTest.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/ClientBase.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/SessionTrackerCheckTest.java

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development