Details

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

      Description

      If a server connects to the leader as follower, it will be allowed to vote (with QuorumMaj) even if it is not a follower in the current configuration,
      as the leader does not care who sends the ACK - it only counts the number of ACKs.

        Issue Links

          Activity

          Hide
          Alexander Shraer added a comment -

          I fixed this as part of ZOOKEEPER-107, which required also other changes to the quorum verifiers

          Show
          Alexander Shraer added a comment - I fixed this as part of ZOOKEEPER-107 , which required also other changes to the quorum verifiers
          Hide
          Alexander Shraer added a comment -

          the patch is attached to the jira. It includes a simple fix in QuorumMaj.java. QuorumMaj constructor parameter changed, which required a change where it is used. Created a new test QuorumMajorityTest.java

          Show
          Alexander Shraer added a comment - the patch is attached to the jira. It includes a simple fix in QuorumMaj.java. QuorumMaj constructor parameter changed, which required a change where it is used. Created a new test QuorumMajorityTest.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/12484845/ZOOKEEPER-1113.patch
          against trunk revision 1141746.

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

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

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

          Hi Alex, We tried to make the computation of containsQuorum as simple as possible to be efficient, but it is right that it has the drawback you point out. Also, we haven't measured the performance impact, so in reality, we don't know if having a loop and checking every vote would make any noticeable difference.

          I checked that core test failure is not due to this patch. I'll be reviewing this patch shortly.

          Show
          Flavio Junqueira added a comment - Hi Alex, We tried to make the computation of containsQuorum as simple as possible to be efficient, but it is right that it has the drawback you point out. Also, we haven't measured the performance impact, so in reality, we don't know if having a loop and checking every vote would make any noticeable difference. I checked that core test failure is not due to this patch. I'll be reviewing this patch shortly.
          Hide
          Alexander Shraer added a comment -

          subtask of ZooKeeper-107

          Show
          Alexander Shraer added a comment - subtask of ZooKeeper-107
          Hide
          Flavio Junqueira added a comment -

          The patch attached looks mostly good to me. It needs a couple of small fixes, though. It does not apply cleaning to trunk and the spacing is not right in QuorumMajorityTest:

          +    	//setup servers 1-3 to be followers and 4 and 5 to be observers
          +    	setUp(true);
          +        ackSet.clear();
          +        
          +        // 1 follower out of 3 is not a majority 
          
          Show
          Flavio Junqueira added a comment - The patch attached looks mostly good to me. It needs a couple of small fixes, though. It does not apply cleaning to trunk and the spacing is not right in QuorumMajorityTest: + //setup servers 1-3 to be followers and 4 and 5 to be observers + setUp(true); + ackSet.clear(); + + // 1 follower out of 3 is not a majority
          Hide
          Alexander Shraer added a comment -

          thanks Flavio, this is actually subsumed by 1411. I will fix the spacing in QuorumMajorityTest and move it to 1411.

          Show
          Alexander Shraer added a comment - thanks Flavio, this is actually subsumed by 1411. I will fix the spacing in QuorumMajorityTest and move it to 1411.
          Hide
          Alexander Shraer added a comment -

          after looking more closely on this, QuorumMajorityTest requires changes to Proposal that I didn't include in 1411. It will go into the next reconfiguration-related patch.

          Show
          Alexander Shraer added a comment - after looking more closely on this, QuorumMajorityTest requires changes to Proposal that I didn't include in 1411. It will go into the next reconfiguration-related patch.
          Hide
          Alexander Shraer added a comment -

          The fix to this is in ZOOKEEPER-107. The idea is to check if an ack is valid when the ack is received. Then, the quorumVerifier can
          still check just the number of acks (from the set of valid acks). QuorumMajorityTest in ZK-107 is a new test to verify that this works.

          Show
          Alexander Shraer added a comment - The fix to this is in ZOOKEEPER-107 . The idea is to check if an ack is valid when the ack is received. Then, the quorumVerifier can still check just the number of acks (from the set of valid acks). QuorumMajorityTest in ZK-107 is a new test to verify that this works.
          Hide
          Alexander Shraer added a comment -

          part of ZOOKEEPER-107

          Show
          Alexander Shraer added a comment - part of ZOOKEEPER-107

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development