Details

    • Type: Sub-task Sub-task
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.5.0
    • Component/s: java client
    • Labels:
      None

      Description

      As part of making ZooKeeper clients more test friendly, it would be useful to easily simulate a session loss event

      1. ZOOKEEPER-1730-8.patch
        7 kB
        Jordan Zimmerman
      2. gitdiff
        0.6 kB
        Thawan Kooburat

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          SUCCESS: Integrated in ZooKeeper-trunk #2276 (See https://builds.apache.org/job/ZooKeeper-trunk/2276/)
          ZOOKEEPER-1730. Make ZooKeeper easier to test - support simulating a session expiration (Jordan Zimmerman via michim) (michim: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1583513)

          • /zookeeper/trunk/CHANGES.txt
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/ClientCnxn.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/Testable.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/ZooKeeper.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/ZooKeeperTestable.java
          • /zookeeper/trunk/src/java/test/org/apache/zookeeper/ZooKeeperTestableTest.java
          Show
          Hudson added a comment - SUCCESS: Integrated in ZooKeeper-trunk #2276 (See https://builds.apache.org/job/ZooKeeper-trunk/2276/ ) ZOOKEEPER-1730 . Make ZooKeeper easier to test - support simulating a session expiration (Jordan Zimmerman via michim) (michim: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1583513 ) /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/src/java/main/org/apache/zookeeper/ClientCnxn.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/Testable.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/ZooKeeper.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/ZooKeeperTestable.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/ZooKeeperTestableTest.java
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12597647/ZOOKEEPER-1730-8.patch
          against trunk revision 1583509.

          +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/2015//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2015//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2015//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/12597647/ZOOKEEPER-1730-8.patch against trunk revision 1583509. +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/2015//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2015//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2015//console This message is automatically generated.
          Show
          Michi Mutsuzaki added a comment - trunk: http://svn.apache.org/viewvc?view=revision&revision=1583513
          Hide
          Michi Mutsuzaki added a comment -

          +1 Thank you Jordan, I'm checking this in.

          Show
          Michi Mutsuzaki added a comment - +1 Thank you Jordan, I'm checking this in.
          Hide
          Jordan Zimmerman added a comment -

          Any love for this patch?

          Show
          Jordan Zimmerman added a comment - Any love for this 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/12597647/ZOOKEEPER-1730-8.patch
          against trunk revision 1503101.

          +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/1538//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1538//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1538//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/12597647/ZOOKEEPER-1730-8.patch against trunk revision 1503101. +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/1538//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1538//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1538//console This message is automatically generated.
          Hide
          Jordan Zimmerman added a comment -

          Added missing license headers

          Show
          Jordan Zimmerman added a comment - Added missing license headers
          Hide
          Jordan Zimmerman added a comment -

          Added missing license headers

          Show
          Jordan Zimmerman added a comment - Added missing license headers
          Hide
          Jordan Zimmerman added a comment -

          Don't know why it got a -1 - looks like it worked

          Show
          Jordan Zimmerman added a comment - Don't know why it got a -1 - looks like it worked
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12597638/ZOOKEEPER-1730-7.patch
          against trunk revision 1503101.

          +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 generated 27 release audit warnings (more than the trunk's current 26 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/1537//testReport/
          Release audit warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1537//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1537//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1537//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/12597638/ZOOKEEPER-1730-7.patch against trunk revision 1503101. +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 generated 27 release audit warnings (more than the trunk's current 26 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/1537//testReport/ Release audit warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1537//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1537//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1537//console This message is automatically generated.
          Hide
          Jordan Zimmerman added a comment -

          I think this will work

          Show
          Jordan Zimmerman added a comment - I think this will work
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12597629/ZOOKEEPER-1730-6.patch
          against trunk revision 1503101.

          +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/1536//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/12597629/ZOOKEEPER-1730-6.patch against trunk revision 1503101. +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/1536//console This message is automatically generated.
          Hide
          Jordan Zimmerman added a comment -

          Needs to be an svn formatted patch. Trying once more.

          Show
          Jordan Zimmerman added a comment - Needs to be an svn formatted patch. Trying once more.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12597625/ZOOKEEPER-1730-5.patch
          against trunk revision 1503101.

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

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

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

          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1535//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/12597625/ZOOKEEPER-1730-5.patch against trunk revision 1503101. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1535//console This message is automatically generated.
          Hide
          Jordan Zimmerman added a comment -

          Wasn't apply-able - fixed

          Show
          Jordan Zimmerman added a comment - Wasn't apply-able - 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/12597624/ZOOKEEPER-1730-4.patch
          against trunk revision 1503101.

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

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

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

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

          Updated patch. Includes recommended renaming and removes unneeded CountDownLatch in ClientCnxn.

          Show
          Jordan Zimmerman added a comment - Updated patch. Includes recommended renaming and removes unneeded CountDownLatch in ClientCnxn.
          Hide
          Jordan Zimmerman added a comment -

          The SendThread does not exit when there is a Disconnect. Disconnect is totally different than session expiration. I've debugged heavily through that code. When there is a connection problem, any pending packets get set to connectionloss but the SendThread does not exist.

          Show
          Jordan Zimmerman added a comment - The SendThread does not exit when there is a Disconnect. Disconnect is totally different than session expiration. I've debugged heavily through that code. When there is a connection problem, any pending packets get set to connectionloss but the SendThread does not exist.
          Hide
          Thawan Kooburat added a comment -

          Sorry, I just tested your patch manually. Here is what i see

          In this case, when you caused the SendThread to exit, it will be the same as trigger connection loss. On exit, SendThread will queue connection loss event if the state.isAlive() is true (but it isn't in the current patch)

          So if you call sendThread.join() before queuing session expire event and modify the SendThread exit path a bit we should get disconnect event and then followed by session expire.

          So this is why I think it is a just a bit more effort to add injectDiscconnect() into this patch.

          Show
          Thawan Kooburat added a comment - Sorry, I just tested your patch manually. Here is what i see In this case, when you caused the SendThread to exit, it will be the same as trigger connection loss. On exit, SendThread will queue connection loss event if the state.isAlive() is true (but it isn't in the current patch) So if you call sendThread.join() before queuing session expire event and modify the SendThread exit path a bit we should get disconnect event and then followed by session expire. So this is why I think it is a just a bit more effort to add injectDiscconnect() into this patch.
          Hide
          Jordan Zimmerman added a comment -

          "check that we get disconnected and then followed by session expired event" - that's not correct as I understand it. The watcher should only get Event.KeeperState.Expired. Can you explain why it should get two events?

          Show
          Jordan Zimmerman added a comment - "check that we get disconnected and then followed by session expired event" - that's not correct as I understand it. The watcher should only get Event.KeeperState.Expired. Can you explain why it should get two events?
          Hide
          Thawan Kooburat added a comment -

          Comments:
          1. instead of using debugSendThreadExitLatch. You should be able to just call clientCnxn.sendThread.join() to wait for sendThread to exit inside injectSessionExpiration()

          2. Can you rename TestTestable to be something like ZooKeeperTestableTest. This seem to be a naming conversion for the rest of the test cases

          3. in unit test, it will be great if we you can also check that we get disconnected and then followed by session expired event.

          The rest looks good to me.

          Show
          Thawan Kooburat added a comment - Comments: 1. instead of using debugSendThreadExitLatch. You should be able to just call clientCnxn.sendThread.join() to wait for sendThread to exit inside injectSessionExpiration() 2. Can you rename TestTestable to be something like ZooKeeperTestableTest. This seem to be a naming conversion for the rest of the test cases 3. in unit test, it will be great if we you can also check that we get disconnected and then followed by session expired event. The rest looks good to me.
          Hide
          Thawan Kooburat added a comment -

          I used to script to convert git patch to svn patch. This only works if you are using git to clone from SVN repo.

          If you are cloning from git repo, you might be able to find the revision number and hard code it in the script.

          Show
          Thawan Kooburat added a comment - I used to script to convert git patch to svn patch. This only works if you are using git to clone from SVN repo. If you are cloning from git repo, you might be able to find the revision number and hard code it in the script.
          Hide
          Thawan Kooburat added a comment -

          cancel it since this is a git patch

          Show
          Thawan Kooburat added a comment - cancel it since this is a git patch
          Hide
          Thawan Kooburat added a comment -

          Kicking Hadoop QA to run

          Show
          Thawan Kooburat added a comment - Kicking Hadoop QA to run
          Hide
          Jordan Zimmerman added a comment -

          1. Updated to block until SendThread exists. 2. Test checks that watchers fire

          Show
          Jordan Zimmerman added a comment - 1. Updated to block until SendThread exists. 2. Test checks that watchers fire
          Hide
          Jordan Zimmerman added a comment -

          > Unit test should make sure that ZooKeeperTestable behave as expected
          > regarding state transition and pending operations/callbacks
          For this, I'd add a watcher and make sure it gets signaled, right? What other checks should the test do?

          Show
          Jordan Zimmerman added a comment - > Unit test should make sure that ZooKeeperTestable behave as expected > regarding state transition and pending operations/callbacks For this, I'd add a watcher and make sure it gets signaled, right? What other checks should the test do?
          Hide
          Jordan Zimmerman added a comment -

          Let's do injectConnectionLoss() as a separate task.

          Show
          Jordan Zimmerman added a comment - Let's do injectConnectionLoss() as a separate task.
          Hide
          Thawan Kooburat added a comment -

          This looks good. I think we can put a bit more effort to make this mimic a real behavior.

          I think the important part is that we need to fail all pending request and callback correctly (SendThread.cleanup()). Additionally, the client should see the state transition in order disconnected -> session expire. So user can write unit test based on what ZooKeeeper client guarantee.

          Here are my suggestions

          1. We can add injectConnectionLoss() method. Essentially, we need SendThread to call cleanup() and queue Disconnected event into the queue. We might be able to do this by causing SendThread to exit

          2. In injectSessionExpire() method. I think the current approach is good, but we should wait until SendThread exit before queuing SessionExpire event. So it is like invoking injectConnectionLoss() if it isn't already in disconnected state.

          3. Unit test should make sure that ZooKeeperTestable behave as expected regarding state transition and pending operations/callbacks.

          Show
          Thawan Kooburat added a comment - This looks good. I think we can put a bit more effort to make this mimic a real behavior. I think the important part is that we need to fail all pending request and callback correctly (SendThread.cleanup()). Additionally, the client should see the state transition in order disconnected -> session expire. So user can write unit test based on what ZooKeeeper client guarantee. Here are my suggestions 1. We can add injectConnectionLoss() method. Essentially, we need SendThread to call cleanup() and queue Disconnected event into the queue. We might be able to do this by causing SendThread to exit 2. In injectSessionExpire() method. I think the current approach is good, but we should wait until SendThread exit before queuing SessionExpire event. So it is like invoking injectConnectionLoss() if it isn't already in disconnected state. 3. Unit test should make sure that ZooKeeperTestable behave as expected regarding state transition and pending operations/callbacks.
          Hide
          Jordan Zimmerman added a comment -

          Oops - sorry about that. Here's the correct patch.

          Show
          Jordan Zimmerman added a comment - Oops - sorry about that. Here's the correct patch.
          Hide
          Thawan Kooburat added a comment -

          Did you for got to add new files to the patch?

          Show
          Thawan Kooburat added a comment - Did you for got to add new files to the patch?
          Hide
          Jordan Zimmerman added a comment -

          Adds the session killing method plus a simple API for adding more testing methods

          Show
          Jordan Zimmerman added a comment - Adds the session killing method plus a simple API for adding more testing methods

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development