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

        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.
        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.
        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
        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.
        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.
        Hide
        Mahadev konar added a comment -

        Rerunning through hudson.

        Show
        Mahadev konar added a comment - Rerunning through hudson.
        Hide
        Mahadev konar added a comment -

        Re uploading the patch for hudson.

        Show
        Mahadev konar added a comment - Re uploading the patch for hudson.
        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.
        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.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development