ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-1136

NEW_LEADER should be queued not sent to match the Zab 1.0 protocol on the twiki

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.4.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      the NEW_LEADER message was sent at the beginning of the sync phase in Zab pre1.0, but it must be at the end in Zab 1.0. if the protocol is 1.0 or greater we need to queue rather than send the packet.

      1. ZOOKEEPER-1136.patch
        35 kB
        Benjamin Reed
      2. ZOOKEEPER-1136.patch
        9 kB
        Benjamin Reed
      3. ZOOKEEPER-1136.patch
        8 kB
        Benjamin Reed

        Activity

        Hide
        Mahadev konar added a comment -

        Ben, are you working on this?

        Show
        Mahadev konar added a comment - Ben, are you working on this?
        Hide
        Benjamin Reed added a comment -

        yes, hoping to have a patch today.

        Show
        Benjamin Reed added a comment - yes, hoping to have a patch today.
        Hide
        Hadoop QA added a comment -

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

        +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 failed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/454//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/454//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/454//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/12490258/ZOOKEEPER-1136.patch against trunk revision 1156845. +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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/454//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/454//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/454//console This message is automatically generated.
        Hide
        Mahadev konar added a comment -

        ben, could you please take a look at the failing test?

        Show
        Mahadev konar added a comment - ben, could you please take a look at the failing test?
        Hide
        Benjamin Reed added a comment -

        the LETest had a tick time of 2ms, which is much too small. since the NEWLEADER is at the end of the state transfer rather than the beginning, it takes slightly longer. (evidently more than 4ms on slow machines.)

        Show
        Benjamin Reed added a comment - the LETest had a tick time of 2ms, which is much too small. since the NEWLEADER is at the end of the state transfer rather than the beginning, it takes slightly longer. (evidently more than 4ms on slow machines.)
        Hide
        Hadoop QA added a comment -

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

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

        Great that you've been able to spot the problem, Ben. A few quick comments:

        1. I don't understand why you're checking if a snapshot has been taken. Is it for backward compatibility?
        2. There are two blocks in LearnerHandler that seem to be doing only reformatting. Do we want to have those changes too?
        3. There is no test, but it sounds like a good idea to have one, no?
        Show
        Flavio Junqueira added a comment - Great that you've been able to spot the problem, Ben. A few quick comments: I don't understand why you're checking if a snapshot has been taken. Is it for backward compatibility? There are two blocks in LearnerHandler that seem to be doing only reformatting. Do we want to have those changes too? There is no test, but it sounds like a good idea to have one, no?
        Hide
        Benjamin Reed added a comment -

        1. yes, i'll put a comment to that effect in the patch
        2. it is only reformatting. the previous patch was checked in with tabs
        3. yes, we should add a test. let me see what i can do.

        Show
        Benjamin Reed added a comment - 1. yes, i'll put a comment to that effect in the patch 2. it is only reformatting. the previous patch was checked in with tabs 3. yes, we should add a test. let me see what i can do.
        Hide
        Mahadev konar added a comment -

        Ben,
        Any update on this?

        Show
        Mahadev konar added a comment - Ben, Any update on this?
        Hide
        Benjamin Reed added a comment -

        Added tests and comments suggested by flavio

        Show
        Benjamin Reed added a comment - Added tests and comments suggested by flavio
        Hide
        Hadoop QA added a comment -

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

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

        +1 tests included. The patch appears to include 2 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/498//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/498//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/498//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/12492950/ZOOKEEPER-1136.patch against trunk revision 1164758. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 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/498//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/498//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/498//console This message is automatically generated.
        Hide
        Mahadev konar added a comment - - edited

        The patch looks good to me, but there are changes that werent necessary in the patch, like:

        +            sock.setSoTimeout(leader.self.getTickTime()*leader.self.getInitLimit());
        

        in LearnerHandler.java and also the ByteBufferOutputStream changes.

        We should enumerate the list of changes made in a patch so that its easier for others to reason about the changes.

        I will go ahead and commit this for now.

        Show
        Mahadev konar added a comment - - edited The patch looks good to me, but there are changes that werent necessary in the patch, like: + sock.setSoTimeout(leader.self.getTickTime()*leader.self.getInitLimit()); in LearnerHandler.java and also the ByteBufferOutputStream changes. We should enumerate the list of changes made in a patch so that its easier for others to reason about the changes. I will go ahead and commit this for now.
        Hide
        Mahadev konar added a comment -

        Just committed this to trunk only. The patch doesnt apply to 3.3 branch. Also, I think its a tricky change for the 3.3 branch. We should skip this change in 3.3.

        Show
        Mahadev konar added a comment - Just committed this to trunk only. The patch doesnt apply to 3.3 branch. Also, I think its a tricky change for the 3.3 branch. We should skip this change in 3.3.
        Hide
        Hudson added a comment -

        Integrated in ZooKeeper-trunk #1304 (See https://builds.apache.org/job/ZooKeeper-trunk/1304/)
        ZOOKEEPER-1136. NEW_LEADER should be queued not sent to match the Zab 1.0 protocol on the twiki (breed via mahadev)

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

        • /zookeeper/trunk/CHANGES.txt
        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ByteBufferInputStream.java
        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ByteBufferOutputStream.java
        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/DataTree.java
        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java
        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java
        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java
        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/Leader.java
        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/Learner.java
        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java
        • /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java
        Show
        Hudson added a comment - Integrated in ZooKeeper-trunk #1304 (See https://builds.apache.org/job/ZooKeeper-trunk/1304/ ) ZOOKEEPER-1136 . NEW_LEADER should be queued not sent to match the Zab 1.0 protocol on the twiki (breed via mahadev) mahadev : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1170452 Files : /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ByteBufferInputStream.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ByteBufferOutputStream.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/DataTree.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/Leader.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/Learner.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java
        Hide
        Camille Fournier added a comment -

        This change causes a concurrency bug. Specifically:
        1. Follower rejoins, gets snap from leader
        2. Follower gets NEWLEADER message and takes a snapshot
        3. Follower gets some additional tranactions forwarded from leader, applies these directly to data tree
        4. Follower gets an UPTODATE message, does not take a snapshot
        5. Follower starts following, writes some new transactions to its log, and is killed before it takes another snapshot
        6. Follower restarts and gets a DIFF from the leader

        The transactions that came in between NEWLEADER and UPTODATE are lost because they never go anywhere but the internal data tree, and if that tree isn't snapshotted and the follower restarts with only a DIFF, the follower will lose these transactions.

        I think the proper thing to do is snapshot after UPTODATE, but I'm not sure why we changed this to snapshot after NEWLEADER instead. The wiki doesn't seem to explain that clearly. If one of you could check on https://issues.apache.org/jira/browse/ZOOKEEPER-1264 and let me know the reasoning, that would be helpful.

        Show
        Camille Fournier added a comment - This change causes a concurrency bug. Specifically: 1. Follower rejoins, gets snap from leader 2. Follower gets NEWLEADER message and takes a snapshot 3. Follower gets some additional tranactions forwarded from leader, applies these directly to data tree 4. Follower gets an UPTODATE message, does not take a snapshot 5. Follower starts following, writes some new transactions to its log, and is killed before it takes another snapshot 6. Follower restarts and gets a DIFF from the leader The transactions that came in between NEWLEADER and UPTODATE are lost because they never go anywhere but the internal data tree, and if that tree isn't snapshotted and the follower restarts with only a DIFF, the follower will lose these transactions. I think the proper thing to do is snapshot after UPTODATE, but I'm not sure why we changed this to snapshot after NEWLEADER instead. The wiki doesn't seem to explain that clearly. If one of you could check on https://issues.apache.org/jira/browse/ZOOKEEPER-1264 and let me know the reasoning, that would be helpful.

          People

          • Assignee:
            Benjamin Reed
            Reporter:
            Benjamin Reed
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development