ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-1192

Leader.waitForEpochAck() checks waitingForNewEpoch instead of checking electionFinished

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.4.0, 3.5.0
    • Component/s: server
    • Labels:
      None

      Description

      A follower/leader should block in Leader.waitForEpochAck() until either electingFollowers contains a quorum and electionFinished=true or until a timeout occurs. A timeout means that a quorum of followers didn't ack the epoch on time, which is an error.

      But the check in Leader.waitForEpochAck() is "if (waitingForNewEpoch) throw..." and this will never be triggered, even if the wait statement just timed out, because Leader.getEpochToPropose() completes and sets waitingForNewEpoch to false before Leader.waitForEpochAck() is invoked.

      Instead of "if (waitingForNewEpoch) throw" the condition in Leader.waitForEpochAck() should be "if (!electionFinished) throw".
      The guarded block introduced in ZK-1191 should be checking !electionFinished.

      1. zookeeper-1192.patch
        2 kB
        Alexander Shraer
      2. ZOOKEEPER-1192.patch
        6 kB
        Benjamin Reed
      3. zookeeper-1192-ver1.patch
        6 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/12495007/zookeeper-1192.patch
        against trunk revision 1172406.

        +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 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/558//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/558//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/558//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/12495007/zookeeper-1192.patch against trunk revision 1172406. +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 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/558//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/558//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/558//console This message is automatically generated.
        Hide
        Patrick Hunt added a comment -

        It would be good to have a test for this.

        Show
        Patrick Hunt added a comment - It would be good to have a test for this.
        Hide
        Alexander Shraer added a comment -

        Includes a test that makes sure that leader doesn't adopt a new epoch that it proposed if it didn't get a quorum of followers to ack this epoch.

        Show
        Alexander Shraer added a comment - Includes a test that makes sure that leader doesn't adopt a new epoch that it proposed if it didn't get a quorum of followers to ack this epoch.
        Hide
        Alexander Shraer added a comment -

        the new test is Zab1_0Test.testAbandonBeforeACKEpoch(). It fails without the patch and passes with the patch.

        Show
        Alexander Shraer added a comment - the new test is Zab1_0Test.testAbandonBeforeACKEpoch(). It fails without the patch and passes with the 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/12495146/zookeeper-1192-ver1.patch
        against trunk revision 1172406.

        +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/564//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/564//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/564//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/12495146/zookeeper-1192-ver1.patch against trunk revision 1172406. +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/564//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/564//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/564//console This message is automatically generated.
        Hide
        Benjamin Reed added a comment -

        +1 looks great alex. i think it is ready to commit. do you want it committed to the 3.4 branch mahadev?

        Show
        Benjamin Reed added a comment - +1 looks great alex. i think it is ready to commit. do you want it committed to the 3.4 branch mahadev?
        Hide
        Benjamin Reed added a comment -

        the patch with the tabs and ^Ms fixed

        Show
        Benjamin Reed added a comment - the patch with the tabs and ^Ms 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/12496069/ZOOKEEPER-1192.patch
        against trunk revision 1172406.

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

        Integrated in ZooKeeper-trunk #1311 (See https://builds.apache.org/job/ZooKeeper-trunk/1311/)
        ZOOKEEPER-1191. Synchronization issue - wait not in guarded block
        ZOOKEEPER-1192. Leader.waitForEpochAck() checks waitingForNewEpoch instead of checking electionFinished

        breed : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1173949
        Files :

        • /zookeeper/trunk/CHANGES.txt
        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/Leader.java
        • /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java
        Show
        Hudson added a comment - Integrated in ZooKeeper-trunk #1311 (See https://builds.apache.org/job/ZooKeeper-trunk/1311/ ) ZOOKEEPER-1191 . Synchronization issue - wait not in guarded block ZOOKEEPER-1192 . Leader.waitForEpochAck() checks waitingForNewEpoch instead of checking electionFinished breed : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1173949 Files : /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/Leader.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java
        Hide
        Camille Fournier added a comment -

        Can this issue be closed?

        Show
        Camille Fournier added a comment - Can this issue be closed?
        Hide
        Alexander Shraer added a comment -

        Integrated in ZooKeeper-trunk #1311 (See https://builds.apache.org/job/ZooKeeper-trunk/1311/)
        ZOOKEEPER-1191. Synchronization issue - wait not in guarded block
        ZOOKEEPER-1192. Leader.waitForEpochAck() checks waitingForNewEpoch instead of checking electionFinished

        Show
        Alexander Shraer added a comment - Integrated in ZooKeeper-trunk #1311 (See https://builds.apache.org/job/ZooKeeper-trunk/1311/ ) ZOOKEEPER-1191 . Synchronization issue - wait not in guarded block ZOOKEEPER-1192 . Leader.waitForEpochAck() checks waitingForNewEpoch instead of checking electionFinished
        Hide
        Camille Fournier added a comment -

        Why wasn't this committed to 3.4? This seems like something that is definitely needed for future patches to work. Specifically, I'm having quite a time with 1264 and I suspect we'll have issues with 1270/1194. We really need to get some coherence here, bugs in election logic should absolutely be going into the 3.4 branch, not just trunk.

        Show
        Camille Fournier added a comment - Why wasn't this committed to 3.4? This seems like something that is definitely needed for future patches to work. Specifically, I'm having quite a time with 1264 and I suspect we'll have issues with 1270/1194. We really need to get some coherence here, bugs in election logic should absolutely be going into the 3.4 branch, not just trunk.
        Hide
        Camille Fournier added a comment -

        Please make sure this is put in 3.4 as well.

        Show
        Camille Fournier added a comment - Please make sure this is put in 3.4 as well.
        Hide
        Mahadev konar added a comment -

        Just merged this into 3.4 branch. Thanks Alex.

        Show
        Mahadev konar added a comment - Just merged this into 3.4 branch. Thanks Alex.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development