ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-549

Refactor Followers and related classes into a Peer->Follower hierarchy in preparation for Observers

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.2.1
    • Fix Version/s: 3.3.0
    • Component/s: quorum, server
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      Refactor followers code into Learner from which followers and observers will inherit.

      Description

      For the Observers patch (ZOOKEEPER-368), a lot of functionality is shared between Followers and Observers. To avoid copying code, it makes sense to push the common code into a parent Peer class and specialise it for Followers and Observers. At the same time, some of the lengthier methods in Follower can be broken up to make the code more readable.

      1. ZOOKEEPER-549.patch
        108 kB
        Henry Robinson
      2. ZOOKEEPER-549.patch
        108 kB
        Henry Robinson
      3. ZOOKEEPER-549.patch
        106 kB
        Henry Robinson
      4. ZOOKEEPER-549.patch
        105 kB
        Henry Robinson

        Activity

        Hide
        Henry Robinson added a comment -

        This patch refactors Followers into a Peer->Follower hierarchy in preparation for the upcoming Observers patch (ZOOKEEPER-368).

        There's no new functionality in this patch, save for the introduction of the getView API in QuorumPeer which is tested by two tests in QuorumTest. All tests pass when this patch is applied against trunk.

        For ease of review, I've listed the main changes below:

        1. A Peer class has been introduced which is a superclass of Follower (and, eventually, Observer). Functionality common to all peers has been moved into this class.
        2. Follower.followLeader has been refactored into a driver for three methods contained in Peer. These three methods are: connectToLeader(InetSocketAddress), registerWithLeader(int pktType) and syncWithLeader(long newLeaderzxid). Observers will contain their own driver method which calls these three methods in the superclass.
        3. Similarly, FollowerZooKeeperHandler has been refactored into PeerZooKeeperHandler and FollowerZooKeeperHandler.
        4. FollowerHandler has been renamed PeerHandler. The sock variable has been made protected and is accessed via getSocket().
        5. ZooKeeperServer.getTouchSnapshot has been made protected as it is not called outside its own class.
        6. FollowerCnxAcceptor has been renamed PeerCnxAcceptor
        7. QuorumPeer has a new getView API which returns the internally held map of QuorumPeers, plus a viewContains method to determine if a server is in that map.

        Show
        Henry Robinson added a comment - This patch refactors Followers into a Peer->Follower hierarchy in preparation for the upcoming Observers patch ( ZOOKEEPER-368 ). There's no new functionality in this patch, save for the introduction of the getView API in QuorumPeer which is tested by two tests in QuorumTest. All tests pass when this patch is applied against trunk. For ease of review, I've listed the main changes below: 1. A Peer class has been introduced which is a superclass of Follower (and, eventually, Observer). Functionality common to all peers has been moved into this class. 2. Follower.followLeader has been refactored into a driver for three methods contained in Peer. These three methods are: connectToLeader(InetSocketAddress), registerWithLeader(int pktType) and syncWithLeader(long newLeaderzxid). Observers will contain their own driver method which calls these three methods in the superclass. 3. Similarly, FollowerZooKeeperHandler has been refactored into PeerZooKeeperHandler and FollowerZooKeeperHandler. 4. FollowerHandler has been renamed PeerHandler. The sock variable has been made protected and is accessed via getSocket(). 5. ZooKeeperServer.getTouchSnapshot has been made protected as it is not called outside its own class. 6. FollowerCnxAcceptor has been renamed PeerCnxAcceptor 7. QuorumPeer has a new getView API which returns the internally held map of QuorumPeers, plus a viewContains method to determine if a server is in that map.
        Hide
        Hadoop QA added a comment -

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

        +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 appears to introduce 2 new Findbugs 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: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/21/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/21/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/21/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/12421844/ZOOKEEPER-549.patch against trunk revision 823371. +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 appears to introduce 2 new Findbugs 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: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/21/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/21/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/21/console This message is automatically generated.
        Hide
        Henry Robinson added a comment -

        Both findbugs warnings were, I think, present in previous builds - they've just moved.

        One is the use of System.exit(13), and the other is an unused SessionExpirer member.

        Show
        Henry Robinson added a comment - Both findbugs warnings were, I think, present in previous builds - they've just moved. One is the use of System.exit(13), and the other is an unused SessionExpirer member.
        Hide
        Patrick Hunt added a comment -

        No, the current build has 0 findbugs warnings (and we want to keep it that way) – if you use the exclusion list that's in svn.

        It may be the case that the exclusion matcher is no longer matching, if you say, moved some code around. You may need to update
        the findbugs exclusion list as part of the patch.

        Show
        Patrick Hunt added a comment - No, the current build has 0 findbugs warnings (and we want to keep it that way) – if you use the exclusion list that's in svn. It may be the case that the exclusion matcher is no longer matching, if you say, moved some code around. You may need to update the findbugs exclusion list as part of the patch.
        Hide
        Henry Robinson added a comment -

        Updated findbugs.xml to mask these warnings. Findbugs goes back to 0 for me locally.

        Show
        Henry Robinson added a comment - Updated findbugs.xml to mask these warnings. Findbugs goes back to 0 for me locally.
        Hide
        Hadoop QA added a comment -

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

        +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 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: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/22/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/22/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/22/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/12421942/ZOOKEEPER-549.patch against trunk revision 823371. +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 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: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/22/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/22/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/22/console This message is automatically generated.
        Hide
        Flavio Junqueira added a comment -

        Thanks, Henry, the patch looks good. I have a couple of high level comments:

        • I think we should call "Peer*" classes something else because calling them Peer gives the idea that it contains functionality that belongs to leaders, followers, and observers. Leader does not extend Peer, though. Also, we already have a QuorumPeer class (and related classes), which makes it. In my interpretation, the common functionality between followers and observers is that they commit (learn) transactions, so we could call it a Committer or a Learner. I personally prefer the latter, and I'm obviously interested in other suggestions;
        • Most likely this should be a separate jira, but I'd like to propose that split the package structure further, in particular for the quorum package. I suggest: quorum.leader, quorum.peer, quorum.learner, quorum.le.
        Show
        Flavio Junqueira added a comment - Thanks, Henry, the patch looks good. I have a couple of high level comments: I think we should call "Peer*" classes something else because calling them Peer gives the idea that it contains functionality that belongs to leaders, followers, and observers. Leader does not extend Peer, though. Also, we already have a QuorumPeer class (and related classes), which makes it. In my interpretation, the common functionality between followers and observers is that they commit (learn) transactions, so we could call it a Committer or a Learner. I personally prefer the latter, and I'm obviously interested in other suggestions; Most likely this should be a separate jira, but I'd like to propose that split the package structure further, in particular for the quorum package. I suggest: quorum.leader, quorum.peer, quorum.learner, quorum.le.
        Hide
        Mahadev konar added a comment -

        henry,
        good to see this. This is much needed refactoring. Some comments/questions-

        • +1 on flavio's suggestion. I couldnt find a better name then Peer though . We do have to keep the Followers as Followers because that we use for the ones that have a say in the proposals!
        • there is an unwanted import in Follower.java and more in other classes like FollowerZooKeepeerServer and maybe others.
            import org.apache.zookeeper.txn.TxnHeader;
            
        • downcast operations in Follower.java for downcasting to FollowerZooKeeperServer. Its done on each and every read of packet and processing. There is some cost to it.

        Downcast operations (also called narrowing conversions in the Java Language Specification) convert an ancestor class reference to a subclass reference. This casting operation creates execution overhead, since Java requires that the cast be checked at runtime to make sure that it's valid.

        from javaworld.
        I think you should be able to downcast once and use that object from then on?

        • other then that, we might want to consider refactoring all the classes into subpackages like flavio suggested
        • also, we should make sure we do enough testing because this is a lot of refactoring.
        Show
        Mahadev konar added a comment - henry, good to see this. This is much needed refactoring. Some comments/questions- +1 on flavio's suggestion. I couldnt find a better name then Peer though . We do have to keep the Followers as Followers because that we use for the ones that have a say in the proposals! there is an unwanted import in Follower.java and more in other classes like FollowerZooKeepeerServer and maybe others. import org.apache.zookeeper.txn.TxnHeader; downcast operations in Follower.java for downcasting to FollowerZooKeeperServer. Its done on each and every read of packet and processing. There is some cost to it. Downcast operations (also called narrowing conversions in the Java Language Specification) convert an ancestor class reference to a subclass reference. This casting operation creates execution overhead, since Java requires that the cast be checked at runtime to make sure that it's valid. from javaworld. I think you should be able to downcast once and use that object from then on? other then that, we might want to consider refactoring all the classes into subpackages like flavio suggested also, we should make sure we do enough testing because this is a lot of refactoring.
        Hide
        Henry Robinson added a comment -

        Mahadev / Flavio -

        Thanks for the comments!

        I agree about renaming from Peer; Learner isn't a bad name. If no-one has a better suggestion, I'll do that

        Good point on the downcasts, I'll remove the unnecessary ones. Also good catch on the imports; I rely on Eclipse to catch them and it sometimes doesn't.

        Definitely agree on the testing; but since this is a rearrangement of code it's not clear how to write many more functional tests since we already have some coverage. Is anyone running a stress test workload? I've just got my hands on some resources to do something similar, but setting it up would take a bit of time.

        I think there are a host of refactorings that could still stand to be done on this code and elsewhere. I agree on the package split, that probably belongs in this JIRA.

        Show
        Henry Robinson added a comment - Mahadev / Flavio - Thanks for the comments! I agree about renaming from Peer; Learner isn't a bad name. If no-one has a better suggestion, I'll do that Good point on the downcasts, I'll remove the unnecessary ones. Also good catch on the imports; I rely on Eclipse to catch them and it sometimes doesn't. Definitely agree on the testing; but since this is a rearrangement of code it's not clear how to write many more functional tests since we already have some coverage. Is anyone running a stress test workload? I've just got my hands on some resources to do something similar, but setting it up would take a bit of time. I think there are a host of refactorings that could still stand to be done on this code and elsewhere. I agree on the package split, that probably belongs in this JIRA.
        Hide
        Henry Robinson added a comment -

        This patch renames the Peer* classes to Learner*, fixes the downcast issue and removes some unused imports. I haven't moved the packages around, just to keep this patch manageable.

        Show
        Henry Robinson added a comment - This patch renames the Peer* classes to Learner*, fixes the downcast issue and removes some unused imports. I haven't moved the packages around, just to keep this patch manageable.
        Hide
        Hadoop QA added a comment -

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

        +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 appears to introduce 2 new Findbugs 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: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/32/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/32/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/32/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/12422770/ZOOKEEPER-549.patch against trunk revision 826787. +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 appears to introduce 2 new Findbugs 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: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/32/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/32/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/32/console This message is automatically generated.
        Hide
        Henry Robinson added a comment -

        Argh, renamed the files that the findbugs exclusions were in, but didn't update the findbugs xml file. Sorry, hudson, try again.

        Show
        Henry Robinson added a comment - Argh, renamed the files that the findbugs exclusions were in, but didn't update the findbugs xml file. Sorry, hudson, try again.
        Hide
        Hadoop QA added a comment -

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

        +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 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: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/33/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/33/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/33/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/12422775/ZOOKEEPER-549.patch against trunk revision 826787. +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 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: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/33/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/33/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/33/console This message is automatically generated.
        Hide
        Flavio Junqueira added a comment -

        Nice job, Henry! I have just a quick question about FOLLOWERINFO. I've noticed that in your patch one of the Learner methods is registerWithLeader(), and this method takes a packet type. I'm assuming that you have not replaced FOLLOWERINFO with LEARNERINFO, and that registerWithLeader takes a packet type as parameter because observers will send something like OBSERVERINFO?

        Assuming that what I said is correct, what if learners send LEARNERINFO, and the leader verifies the status of the server against its configuration?

        I apologize if I'm mixing jiras here. I'm just trying to understand why FOLLOWERINFO hasn'changed.

        Show
        Flavio Junqueira added a comment - Nice job, Henry! I have just a quick question about FOLLOWERINFO. I've noticed that in your patch one of the Learner methods is registerWithLeader(), and this method takes a packet type. I'm assuming that you have not replaced FOLLOWERINFO with LEARNERINFO, and that registerWithLeader takes a packet type as parameter because observers will send something like OBSERVERINFO? Assuming that what I said is correct, what if learners send LEARNERINFO, and the leader verifies the status of the server against its configuration? I apologize if I'm mixing jiras here. I'm just trying to understand why FOLLOWERINFO hasn'changed.
        Hide
        Henry Robinson added a comment -

        You're right - the observers patch will introduce OBSERVERINFO which is the way that the leader learns to distinguish observers from followers.

        It's also possible that the Leader check its configuration and assigns the appropriate role to the Learner. However, I'd still like to have the role negotiated between Learner and Leader and run-time (otherwise we'd have to check for misconfiguration all throughout the code because an observer would silently be treated like a follower, never vote and that would be hard to debug).

        In the observers patch, I'll add some code so that if the Leader receives OBSERVERINFO or FOLLOWERINFO it checks that this is the expected type from that particular peer.

        Show
        Henry Robinson added a comment - You're right - the observers patch will introduce OBSERVERINFO which is the way that the leader learns to distinguish observers from followers. It's also possible that the Leader check its configuration and assigns the appropriate role to the Learner. However, I'd still like to have the role negotiated between Learner and Leader and run-time (otherwise we'd have to check for misconfiguration all throughout the code because an observer would silently be treated like a follower, never vote and that would be hard to debug). In the observers patch, I'll add some code so that if the Leader receives OBSERVERINFO or FOLLOWERINFO it checks that this is the expected type from that particular peer.
        Hide
        Flavio Junqueira added a comment -

        I originally thought that by using LEANERINFO for both observers and followers, we would make the protocol backward compatible. I now realize that it makes no difference.

        Once we add observers, if we have an observer trying to connect to an old leader, even if we use LEANERINFO (with the same value 11 we have for FOLLOWERINFO in trunk), it won't work because the leader will expect acks from that observer. An old server can also connect as a follower to a new leader without a problem if we keep FOLLOWERINFO as is. We would have a problem if the follower were configured as an observer on the leader, but I suppose this is a configuration error.

        My conclusion is that I don't see an issue with using OBSERVERINFO or FOLLOWERINFO regarding backward compatibility. The only issue I see is that having OBSERVERINFO creates yet another message and there is a way around it. I personally don't think it is a big deal to add another message, but some folks might not like it.

        On my end, +1 for the patch. We can continue the discussion on adding OBSERVERINFO or not in the observer jira. For this patch, I think it is good as is.

        Show
        Flavio Junqueira added a comment - I originally thought that by using LEANERINFO for both observers and followers, we would make the protocol backward compatible. I now realize that it makes no difference. Once we add observers, if we have an observer trying to connect to an old leader, even if we use LEANERINFO (with the same value 11 we have for FOLLOWERINFO in trunk), it won't work because the leader will expect acks from that observer. An old server can also connect as a follower to a new leader without a problem if we keep FOLLOWERINFO as is. We would have a problem if the follower were configured as an observer on the leader, but I suppose this is a configuration error. My conclusion is that I don't see an issue with using OBSERVERINFO or FOLLOWERINFO regarding backward compatibility. The only issue I see is that having OBSERVERINFO creates yet another message and there is a way around it. I personally don't think it is a big deal to add another message, but some folks might not like it. On my end, +1 for the patch. We can continue the discussion on adding OBSERVERINFO or not in the observer jira. For this patch, I think it is good as is.
        Hide
        Mahadev konar added a comment -

        the patch looks good but I am getting confused with the OBSERVERINFO/FOLLOWERINFO discussion above... As of this patch is the above discussion relavant? As far as I can tell its just a refactoring and the servers before and after this patch should be able to work with each other ... right?

        Show
        Mahadev konar added a comment - the patch looks good but I am getting confused with the OBSERVERINFO/FOLLOWERINFO discussion above... As of this patch is the above discussion relavant? As far as I can tell its just a refactoring and the servers before and after this patch should be able to work with each other ... right?
        Hide
        Patrick Hunt added a comment -

        To put a fine point on that - ensure that the refactored code is b/w compatible with the original code. That should
        be a general goal as well as an item in the patch reviewers checklist.

        Show
        Patrick Hunt added a comment - To put a fine point on that - ensure that the refactored code is b/w compatible with the original code. That should be a general goal as well as an item in the patch reviewers checklist.
        Hide
        Henry Robinson added a comment -

        Mahadev - yes, the FOLLOWERINFO discussion really applies to the observer patch. I think that we should push that specific backwards compatibility discussion onto the observers jira.

        I believe backwards compatibility is completely preserved by this patch: there aren't any protocol changes.

        Show
        Henry Robinson added a comment - Mahadev - yes, the FOLLOWERINFO discussion really applies to the observer patch. I think that we should push that specific backwards compatibility discussion onto the observers jira. I believe backwards compatibility is completely preserved by this patch: there aren't any protocol changes.
        Hide
        Flavio Junqueira added a comment -

        Just to make my position clear. I initially questioned why the patch doesn't replace FOLLOWERINFO with LEARNERINFO (which would be b/w compatible given that we keep the same value for the constant), and I was trying to understand why Henry didn't do it. After our discussion (above), it became clear that we should leave it for the observer jira.

        Show
        Flavio Junqueira added a comment - Just to make my position clear. I initially questioned why the patch doesn't replace FOLLOWERINFO with LEARNERINFO (which would be b/w compatible given that we keep the same value for the constant), and I was trying to understand why Henry didn't do it. After our discussion (above), it became clear that we should leave it for the observer jira.
        Hide
        Mahadev konar added a comment -

        I tried a bunch of permutations of the new code (after this patch) servers and old code servers just to make sure we are backwards compatible. It all works fine. I will try committing this ASAP after a final review.

        Show
        Mahadev konar added a comment - I tried a bunch of permutations of the new code (after this patch) servers and old code servers just to make sure we are backwards compatible. It all works fine. I will try committing this ASAP after a final review.
        Hide
        Mahadev konar added a comment -

        +1 this looks good.... Ill commit this tonight...

        Show
        Mahadev konar added a comment - +1 this looks good.... Ill commit this tonight...
        Hide
        Mahadev konar added a comment -

        I just committed this. thanks henry.

        Show
        Mahadev konar added a comment - I just committed this. thanks henry.
        Hide
        Hudson added a comment -

        Integrated in ZooKeeper-trunk #514 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/514/)
        . Refactor Followers and related classes into a Peer->Follower hierarchy in preparation for Observers (henry robinson via mahadev)

        Show
        Hudson added a comment - Integrated in ZooKeeper-trunk #514 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/514/ ) . Refactor Followers and related classes into a Peer->Follower hierarchy in preparation for Observers (henry robinson via mahadev)

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development