ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-1094

Small improvements to LeaderElection and Vote classes

    Details

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

      Description

      1. o.a.z.q.Vote is a struct-style class, whose fields are public and not final.

      In general, we should prefer making the fields of these kind of classes final, and hiding them behind getters for the following reasons:

      • Marking them as final allows clients of the class not to worry about any synchronisation when accessing the fields
      • Hiding them behind getters allows us to change the implementation of the class without changing the API.

      Object creation is very cheap. It's ok to create new Votes rather than mutate existing ones.

      2. Votes are mainly used in the LeaderElection class. In this class a map of addresses to votes is passed in to countVotes, which modifies the map contents inside an iterator (and therefore changes the object passed in by reference). This is pretty gross, so at the same time I've slightly refactored this method to return information about the number of validVotes in the ElectionResult class, which is returned by countVotes.

      3. The previous implementation of countVotes was quadratic in the number of votes. It is possible to do this linearly. No real speed-up is expected as a result, but it salves the CS OCD in me

      1. ZK-1094.patch
        33 kB
        Henry Robinson
      2. ZK-1094.patch
        33 kB
        Henry Robinson

        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/12482485/ZK-1094.patch
        against trunk revision 1135270.

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

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

        henry, can you make this patch relative to ZOOKEEPER-335? i plan on committing 335 tonight. i haven't yet learned the git voodoo to do merges.

        Show
        Benjamin Reed added a comment - henry, can you make this patch relative to ZOOKEEPER-335 ? i plan on committing 335 tonight. i haven't yet learned the git voodoo to do merges.
        Hide
        Henry Robinson added a comment -

        Sure - if you commit ZOOKEEPER-335 tonight, I'll rebase against trunk and repost the patch tomorrow. I just read the patch for ZOOKEEPER-335, and there should be very few conflicts. Thanks!

        Show
        Henry Robinson added a comment - Sure - if you commit ZOOKEEPER-335 tonight, I'll rebase against trunk and repost the patch tomorrow. I just read the patch for ZOOKEEPER-335 , and there should be very few conflicts. Thanks!
        Hide
        Benjamin Reed added a comment -

        deal. in return i'll review your patch

        Show
        Benjamin Reed added a comment - deal. in return i'll review your patch
        Hide
        Henry Robinson added a comment -

        Updated patch against trunk, in particular ZOOKEEPER-335.

        Show
        Henry Robinson added a comment - Updated patch against trunk, in particular ZOOKEEPER-335 .
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12482602/ZK-1094.patch
        against trunk revision 1135515.

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

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

        this looks good overall. flavio can you check countVotes() in LeaderElection?

        Show
        Benjamin Reed added a comment - this looks good overall. flavio can you check countVotes() in LeaderElection?
        Hide
        Flavio Junqueira added a comment -

        The improvements look good, but I'm actually not sure why we update all votes for the same id to have the same zxid. It sounds like we are transforming stale votes into valid votes.

        Show
        Flavio Junqueira added a comment - The improvements look good, but I'm actually not sure why we update all votes for the same id to have the same zxid. It sounds like we are transforming stale votes into valid votes.
        Hide
        Henry Robinson added a comment -

        Thanks guys. Regarding the staleness transformation - it did seem a bit weird to me as well. If you think this patch preserves the original behaviour, maybe we should get that committed and open another JIRA to figure out if that behaviour is actually wrong... there are a couple of things in LE that could do with being cleaned up, such as the fact that it doesn't use the current quorum verifier to pass votes.

        Show
        Henry Robinson added a comment - Thanks guys. Regarding the staleness transformation - it did seem a bit weird to me as well. If you think this patch preserves the original behaviour, maybe we should get that committed and open another JIRA to figure out if that behaviour is actually wrong... there are a couple of things in LE that could do with being cleaned up, such as the fact that it doesn't use the current quorum verifier to pass votes.
        Hide
        Flavio Junqueira added a comment -

        +1, it sounds good to me to get these improvements in and have another jira to discuss the staleness issue. On the use of quorum verifier, we have brought up a number of times the option of deprecating LeaderElection (at least I did bring up), and this was one reason at the time we introduced quorum verifiers to not have it in LeaderElection. We should also consider deprecating AuthFLE. We don't even have tests for it, and to my knowledge, no one uses it.

        Show
        Flavio Junqueira added a comment - +1, it sounds good to me to get these improvements in and have another jira to discuss the staleness issue. On the use of quorum verifier, we have brought up a number of times the option of deprecating LeaderElection (at least I did bring up), and this was one reason at the time we introduced quorum verifiers to not have it in LeaderElection. We should also consider deprecating AuthFLE. We don't even have tests for it, and to my knowledge, no one uses it.
        Hide
        Benjamin Reed added a comment -

        +1 ok i'll get this committed. i also think it is the time to move to a single leader election. the current default has been in place a long time, so the caution about moving to it is gone.

        Show
        Benjamin Reed added a comment - +1 ok i'll get this committed. i also think it is the time to move to a single leader election. the current default has been in place a long time, so the caution about moving to it is gone.
        Hide
        Benjamin Reed added a comment -

        Committed revision 1136740.
        thanx henry!

        Show
        Benjamin Reed added a comment - Committed revision 1136740. thanx henry!

          People

          • Assignee:
            Henry Robinson
            Reporter:
            Henry Robinson
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development