ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-1202

Prevent certain state transitions in Java client on close(); improve exception handling and enhance client testability

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 3.4.0
    • Fix Version/s: 3.6.0
    • Component/s: java client
    • Labels:
      None
    • Tags:
      Java client

      Description

      ZooKeeper.close() doesn't force the client into a CLOSED state. While the closing flag ensures that the client will close, its state may end up in CLOSED, CONNECTING or CONNECTED.
      I developed a patch and in the process cleaned up a few other things primarily to enable testing of state transitions.

      • ClientCnxnState is new and enforces certain state transitions
      • ZooKeeper.isExpired() is new
      • ClientCnxn no longer refers to ZooKeeper, WatchManager is externalized, and ClientWatchManager includes 3 new methods
      • The SendThread terminates the EventThread on a call to close() via the event-of-death
      • Polymorphism is used to handle internal exceptions (SendIOExceptions)
      • The patch incorporates ZOOKEEPER-126.patch and prevents close() from blocking
      1. ZOOKEEPER-1202.patch
        75 kB
        Matthias Spycher

        Issue Links

          Activity

          Hide
          Michi Mutsuzaki added a comment -

          The patch doesn't apply anymore.

          I'm pushing this from 3.5 to 3.6.

          Show
          Michi Mutsuzaki added a comment - The patch doesn't apply anymore. I'm pushing this from 3.5 to 3.6.
          Hide
          Hadoop QA added a comment -

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

          +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 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/842//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/842//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/842//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/12508008/ZOOKEEPER-1202.patch against trunk revision 1214571. +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 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/842//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/842//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/842//console This message is automatically generated.
          Hide
          Matthias Spycher added a comment -

          Apply same state check to other tests in ClientCloseTest. Increase timeout to 5s.

          Show
          Matthias Spycher added a comment - Apply same state check to other tests in ClientCloseTest. Increase timeout to 5s.
          Hide
          Hadoop QA added a comment -

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

          +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 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/841//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/841//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/841//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/12507989/ZOOKEEPER-1202.patch against trunk revision 1214571. +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 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/841//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/841//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/841//console This message is automatically generated.
          Hide
          Matthias Spycher added a comment -

          Grant license to ASF.

          Show
          Matthias Spycher added a comment - Grant license to ASF.
          Hide
          Matthias Spycher added a comment -

          Changed ClientCloseTest.testExpiration() to wait for expiration.

          Show
          Matthias Spycher added a comment - Changed ClientCloseTest.testExpiration() to wait for expiration.
          Hide
          Matthias Spycher added a comment -

          Ok, let me change the test a bit to wait until the session is expired. Thanks Camille.

          Show
          Matthias Spycher added a comment - Ok, let me change the test a bit to wait until the session is expired. Thanks Camille.
          Hide
          Camille Fournier added a comment -

          I think you might just need a longer TIMEOUT for that awaitTermination... the thread can sleep for up to 1s in the sendThread run loop before trying to reconnect, so on those slow build machines you might just need a bit more wiggle room. We don't see it even trying to connect until > 2s after the session was closed.

          Show
          Camille Fournier added a comment - I think you might just need a longer TIMEOUT for that awaitTermination... the thread can sleep for up to 1s in the sendThread run loop before trying to reconnect, so on those slow build machines you might just need a bit more wiggle room. We don't see it even trying to connect until > 2s after the session was closed.
          Hide
          Matthias Spycher added a comment -

          ClientCloseTest.testExpiration() is failing here, but I'm unable to reproduce this on my system (Windows 7).

          Patrick, can you take a look and let me know if there's a better way to ensure the server expires the client's session in the testExpiration() method.
          From the logs it appears the SessionExpiredException wasn't thrown on the client-side (which happens every time on my system).
          Thanks.

          Show
          Matthias Spycher added a comment - ClientCloseTest.testExpiration() is failing here, but I'm unable to reproduce this on my system (Windows 7). Patrick, can you take a look and let me know if there's a better way to ensure the server expires the client's session in the testExpiration() method. From the logs it appears the SessionExpiredException wasn't thrown on the client-side (which happens every time on my system). 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/12507940/ZOOKEEPER-1202.patch
          against trunk revision 1214571.

          +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 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/840//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/840//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/840//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/12507940/ZOOKEEPER-1202.patch against trunk revision 1214571. +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 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/840//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/840//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/840//console This message is automatically generated.
          Hide
          Matthias Spycher added a comment -

          Added Apache License comment to new files.

          Show
          Matthias Spycher added a comment - Added Apache License comment to new files.
          Hide
          Hadoop QA added a comment -

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

          +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 1.3.9) warnings.

          -1 release audit. The applied patch generated 28 release audit warnings (more than the trunk's current 24 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/839//testReport/
          Release audit warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/839//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/839//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/839//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/12507933/ZOOKEEPER-1202.patch against trunk revision 1214571. +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 1.3.9) warnings. -1 release audit. The applied patch generated 28 release audit warnings (more than the trunk's current 24 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/839//testReport/ Release audit warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/839//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/839//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/839//console This message is automatically generated.
          Hide
          Matthias Spycher added a comment -

          One more try – it helps to run svn diff in the correct workspace...

          Show
          Matthias Spycher added a comment - One more try – it helps to run svn diff in the correct workspace...
          Hide
          Hadoop QA added a comment -

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

          +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 patch appears to cause tar ant target to fail.

          -1 findbugs. The patch appears to cause Findbugs (version 1.3.9) 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/838//testReport/
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/838//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/12507930/ZOOKEEPER-1202.patch against trunk revision 1214571. +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 patch appears to cause tar ant target to fail. -1 findbugs. The patch appears to cause Findbugs (version 1.3.9) 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/838//testReport/ Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/838//console This message is automatically generated.
          Hide
          Matthias Spycher added a comment -

          Missed a file – trying again.

          Show
          Matthias Spycher added a comment - Missed a file – trying again.
          Hide
          Matthias Spycher added a comment -

          In this latest patch I've left out the polymorphic exception handling for clarity.

          I also ran into some issues with unit tests that I fixed:
          ClientPortBindTest was updated to deal with IPv6 loopback addresses which many cause exceptions on bind().
          LoadFromLogTest had a potential race that has been fixed.

          Show
          Matthias Spycher added a comment - In this latest patch I've left out the polymorphic exception handling for clarity. I also ran into some issues with unit tests that I fixed: ClientPortBindTest was updated to deal with IPv6 loopback addresses which many cause exceptions on bind(). LoadFromLogTest had a potential race that has been fixed.
          Hide
          Hadoop QA added a comment -

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

          +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 patch appears to cause tar ant target to fail.

          -1 findbugs. The patch appears to cause Findbugs (version 1.3.9) 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/837//testReport/
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/837//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/12507927/ZOOKEEPER-1202.patch against trunk revision 1214571. +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 patch appears to cause tar ant target to fail. -1 findbugs. The patch appears to cause Findbugs (version 1.3.9) 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/837//testReport/ Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/837//console This message is automatically generated.
          Hide
          Matthias Spycher added a comment -

          Updated against trunk

          Show
          Matthias Spycher added a comment - Updated against trunk
          Hide
          Patrick Hunt added a comment -

          can you refresh the patch, not applying to trunk.

          Show
          Patrick Hunt added a comment - can you refresh the patch, not applying to trunk.
          Hide
          Hadoop QA added a comment -

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

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

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

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

          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/830//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/12496216/ZOOKEEPER-1202.patch against trunk revision 1214571. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 12 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/830//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/12496216/ZOOKEEPER-1202.patch
          against trunk revision 1173949.

          +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 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/586//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/586//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/586//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/12496216/ZOOKEEPER-1202.patch against trunk revision 1173949. +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 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/586//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/586//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/586//console This message is automatically generated.
          Hide
          Matthias Spycher added a comment -

          Please review

          Show
          Matthias Spycher added a comment - Please review

            People

            • Assignee:
              Matthias Spycher
              Reporter:
              Matthias Spycher
            • Votes:
              1 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:

                Development