ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-1478

Small bug in QuorumTest.testFollowersStartAfterLeader( )

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 3.4.3
    • Fix Version/s: 3.4.6, 3.5.0
    • Component/s: tests
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      The following code appears in QuorumTest.testFollowersStartAfterLeader( ):

      for (int i = 0; i < 30; i++) {
      try

      { zk.create("/test", "test".getBytes(), ZooDefs.Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT); break; }

      catch(KeeperException.ConnectionLossException e)

      { Thread.sleep(1000); }

      // test fails if we still can't connect to the quorum after 30 seconds.
      Assert.fail("client could not connect to reestablished quorum: giving up after 30+ seconds.");
      }

      From the comment it looks like the intention was to try to reconnect 30 times and only then trigger the Assert, but that's not what this does.
      After we fail to connect once and Thread.sleep is executed, Assert.fail will be executed without retrying create.

      1. ZOOKEEPER-1478.patch
        2 kB
        Mahadev konar
      2. ZOOKEEPER-1478.patch
        2 kB
        Alexander Shraer
      3. ZOOKEEPER-1478.patch
        2 kB
        Flavio Junqueira
      4. ZOOKEEPER-1478.patch
        1 kB
        Flavio Junqueira
      5. ZOOKEEPER-1478.patch
        1 kB
        Alexander Shraer

        Activity

        Alexander Shraer created issue -
        Alexander Shraer made changes -
        Field Original Value New Value
        Status Open [ 1 ] Patch Available [ 10002 ]
        Assignee Alexander Shraer [ shralex ]
        Alexander Shraer made changes -
        Attachment ZOOKEEPER-1478.patch [ 12530678 ]
        Hide
        Hadoop QA added a comment -

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

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

        +1 tests included. The patch appears to include 3 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/1086//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1086//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1086//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/12530678/ZOOKEEPER-1478.patch against trunk revision 1337029. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 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/1086//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1086//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1086//console This message is automatically generated.
        Flavio Junqueira made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        Flavio Junqueira made changes -
        Attachment ZOOKEEPER-1478.patch [ 12532571 ]
        Hide
        Flavio Junqueira added a comment -

        Good catch, Alex. I was wondering why we are trying to create znodes instead of trying to simply check if it is connected. Is it because we can have a race and the output of waitForConnected is not reliable? Check the latest patch I have uploaded to see the approach I'm thinking about.

        Other than this issue, the patch you submitted looks good.

        Show
        Flavio Junqueira added a comment - Good catch, Alex. I was wondering why we are trying to create znodes instead of trying to simply check if it is connected. Is it because we can have a race and the output of waitForConnected is not reliable? Check the latest patch I have uploaded to see the approach I'm thinking about. Other than this issue, the patch you submitted looks good.
        Hide
        Alexander Shraer added a comment -

        Hi Flavio, this looks good, although I don't know the details of ZK-790 to which this test is related, so not sure what it is supposed to do exactly. One thing a bit odd is the line

        QuorumBase.waitForServerUp(
        "127.0.0.1:" + qu.getPeer(2).clientPort,
        CONNECTION_TIMEOUT));

        above your changes. Server 2 might not have been even shut down (it should be "index" ?) and in any case your new line waitForConnected seems to be sufficient even without the above line. right ?

        Show
        Alexander Shraer added a comment - Hi Flavio, this looks good, although I don't know the details of ZK-790 to which this test is related, so not sure what it is supposed to do exactly. One thing a bit odd is the line QuorumBase.waitForServerUp( "127.0.0.1:" + qu.getPeer(2).clientPort, CONNECTION_TIMEOUT)); above your changes. Server 2 might not have been even shut down (it should be "index" ?) and in any case your new line waitForConnected seems to be sufficient even without the above line. right ?
        Hide
        Flavio Junqueira added a comment -

        I had a look at ZOOKEEPER-790 and if I checked it right, and this change to run 30 times create was introduced in ZOOKEEPER-1103.

        We are just trying to check if the client is able to reconnect to the ensemble after a leader demotion, so I think the way I'm proposing is cleaner. I'll be uploading shortly a small change that will guarantee that the race I was referring to doesn't happen by waiting until we actually observe that the client has disconnected.

        About the waitForServerUp statement, it doesn't look strictly necessary by the way we create the zookeeper object. It sounds ok to remove.

        Show
        Flavio Junqueira added a comment - I had a look at ZOOKEEPER-790 and if I checked it right, and this change to run 30 times create was introduced in ZOOKEEPER-1103 . We are just trying to check if the client is able to reconnect to the ensemble after a leader demotion, so I think the way I'm proposing is cleaner. I'll be uploading shortly a small change that will guarantee that the race I was referring to doesn't happen by waiting until we actually observe that the client has disconnected. About the waitForServerUp statement, it doesn't look strictly necessary by the way we create the zookeeper object. It sounds ok to remove.
        Hide
        Flavio Junqueira added a comment -

        Change to wait until client actually observes the disconnection.

        Show
        Flavio Junqueira added a comment - Change to wait until client actually observes the disconnection.
        Flavio Junqueira made changes -
        Attachment ZOOKEEPER-1478.patch [ 12532665 ]
        Flavio Junqueira made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        Hadoop QA added a comment -

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

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

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

        removed waitForServerUp

        Show
        Alexander Shraer added a comment - removed waitForServerUp
        Alexander Shraer made changes -
        Attachment ZOOKEEPER-1478.patch [ 12532701 ]
        Hide
        Hadoop QA added a comment -

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

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

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

        This is a +1 for me, but since I added code to the patch, it might be a good idea to have someone else having a look at it too.

        Show
        Flavio Junqueira added a comment - This is a +1 for me, but since I added code to the patch, it might be a good idea to have someone else having a look at it too.
        Patrick Hunt made changes -
        Fix Version/s 3.4.4 [ 12319841 ]
        Fix Version/s 3.5.0 [ 12316644 ]
        Mahadev konar made changes -
        Affects Version/s 3.4.3 [ 12319288 ]
        Hide
        Mahadev konar added a comment -

        Moving it out to 3.5 since the bugfix isnt a really critical one.

        Show
        Mahadev konar added a comment - Moving it out to 3.5 since the bugfix isnt a really critical one.
        Mahadev konar made changes -
        Fix Version/s 3.4.4 [ 12319841 ]
        Hide
        Mahadev konar added a comment -

        Rerunning through hudson.

        Show
        Mahadev konar added a comment - Rerunning through hudson.
        Mahadev konar made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        Hide
        Mahadev konar added a comment -

        Re uploading the patch for hudson.

        Show
        Mahadev konar added a comment - Re uploading the patch for hudson.
        Mahadev konar made changes -
        Attachment ZOOKEEPER-1478.patch [ 12544504 ]
        Mahadev konar made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        Hadoop QA added a comment -

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

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

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

        +1 looks good to me!

        Show
        Benjamin Reed added a comment - +1 looks good to me!
        Hide
        Patrick Hunt added a comment -

        Committed to 3.4.6 and trunk. Thanks Alexander.

        Show
        Patrick Hunt added a comment - Committed to 3.4.6 and trunk. Thanks Alexander.
        Patrick Hunt made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Hadoop Flags Reviewed [ 10343 ]
        Fix Version/s 3.4.6 [ 12323310 ]
        Resolution Fixed [ 1 ]
        Hide
        Hudson added a comment -

        Integrated in ZooKeeper-trunk #1771 (See https://builds.apache.org/job/ZooKeeper-trunk/1771/)
        ZOOKEEPER-1478. Small bug in QuorumTest.testFollowersStartAfterLeader( ) (Alexander Shraer via fpj, breed, phunt) (Revision 1421091)

        Result = SUCCESS
        phunt : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1421091
        Files :

        • /zookeeper/trunk/CHANGES.txt
        • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/QuorumTest.java
        Show
        Hudson added a comment - Integrated in ZooKeeper-trunk #1771 (See https://builds.apache.org/job/ZooKeeper-trunk/1771/ ) ZOOKEEPER-1478 . Small bug in QuorumTest.testFollowersStartAfterLeader( ) (Alexander Shraer via fpj, breed, phunt) (Revision 1421091) Result = SUCCESS phunt : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1421091 Files : /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/QuorumTest.java
        Hide
        Flavio Junqueira added a comment -

        Closing issues after releasing 3.4.6.

        Show
        Flavio Junqueira added a comment - Closing issues after releasing 3.4.6.
        Flavio Junqueira made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Patch Available Patch Available Open Open
        98d 23h 52m 2 Mahadev konar 10/Sep/12 18:51
        Open Open Patch Available Patch Available
        15h 56m 3 Mahadev konar 10/Sep/12 18:52
        Patch Available Patch Available Resolved Resolved
        93d 12h 27m 1 Patrick Hunt 13/Dec/12 07:19
        Resolved Resolved Closed Closed
        455d 10h 57m 1 Flavio Junqueira 13/Mar/14 18:16

          People

          • Assignee:
            Alexander Shraer
            Reporter:
            Alexander Shraer
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development