----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/873/#review790 -----------------------------------------------------------
/src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java <https://reviews.apache.org/r/873/#comment1691> I'm not sure the read means you have a tab there. /src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java <https://reviews.apache.org/r/873/#comment1692> Another red mark. /src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java <https://reviews.apache.org/r/873/#comment1693> This comment is incorrect, it should be peer epoch. /src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java <https://reviews.apache.org/r/873/#comment1694> Another red mark. /src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java <https://reviews.apache.org/r/873/#comment1695> More red (this one was here already apparently). /src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java <https://reviews.apache.org/r/873/#comment1696> We need to fix it to return the value of the current epoch. /src/java/main/org/apache/zookeeper/server/quorum/Follower.java <https://reviews.apache.org/r/873/#comment1697> This is the last red I report. There seem to be a number of them, and I'm not sure what they mean. /src/java/main/org/apache/zookeeper/server/quorum/Leader.java <https://reviews.apache.org/r/873/#comment1698> Formatting seems to be incorrect. /src/java/main/org/apache/zookeeper/server/quorum/Leader.java <https://reviews.apache.org/r/873/#comment1699> If I understand correctly the semantics of this exception thrown here, it is caught in the main try block of LearnerHandler.run(), which causes the handler to shut down. The leader keeps going as long as it is able to get AckEpochs from followers that do not have a more recent epoch. /src/java/main/org/apache/zookeeper/server/quorum/Learner.java <https://reviews.apache.org/r/873/#comment1701> Small comment: We should probably refer to protocol in the name of the variable, something like leaderProtoVersion. /src/java/main/org/apache/zookeeper/server/quorum/Learner.java <https://reviews.apache.org/r/873/#comment1702> This is to tell the leader that it won't accept the epoch, but will follow the leader if it has a quorum, right? I think it would be cool to add a comment here explaining what's going on. I frowned for 10 minutes in front of this line... /src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java <https://reviews.apache.org/r/873/#comment1703> We probably need to document we have these files that the protocol relies upon for correctness. /src/java/main/org/apache/zookeeper/server/quorum/StateSummary.java <https://reviews.apache.org/r/873/#comment1706> There is a findbugs warning about this: HE_EQUALS_USE_HASHCODE: Class defines equals() and uses Object.hashCode() This class overrides equals(Object), but does not override hashCode(), and inherits the implementation of hashCode() from java.lang.Object (which returns the identity hash code, an arbitrary value assigned to the object by the VM). Therefore, the class is very likely to violate the invariant that equal objects must have equal hashcodes. If you don't think instances of this class will ever be inserted into a HashMap/HashTable, the recommended hashCode implementation to use is: public int hashCode() { assert false : "hashCode not designed"; return 42; // any arbitrary constant will do } /src/java/main/org/apache/zookeeper/server/quorum/flexible/QuorumHierarchical.java <https://reviews.apache.org/r/873/#comment1704> Do we want to keep these System.err.println statements? Isn't it better to make them LOG.debug? /src/java/test/org/apache/zookeeper/test/ReadOnlyModeTest.java <https://reviews.apache.org/r/873/#comment1705> I was wondering why you removed this line. I suspect this was causing the bug you mention before, but I was wondering if you have some more insight about it. - fpj On 2011-06-09 14:39:27, fpj wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/873/ > ----------------------------------------------------------- > > (Updated 2011-06-09 14:39:27) > > > Review request for zookeeper, fpj, Vishal Kher, and Benjamin Reed. > > > Summary > ------- > > ZOOKEEPER-335. > > > Diffs > ----- > > /src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java 1132865 > /src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java > 1132865 > /src/java/main/org/apache/zookeeper/server/quorum/Follower.java 1132865 > /src/java/main/org/apache/zookeeper/server/quorum/Leader.java 1132865 > /src/java/main/org/apache/zookeeper/server/quorum/Learner.java 1132865 > /src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java > 1132865 > /src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1132865 > /src/java/main/org/apache/zookeeper/server/quorum/StateSummary.java > PRE-CREATION > /src/java/main/org/apache/zookeeper/server/quorum/Vote.java 1132865 > > /src/java/main/org/apache/zookeeper/server/quorum/flexible/QuorumHierarchical.java > 1132865 > /src/java/main/org/apache/zookeeper/server/util/ZxidUtils.java PRE-CREATION > /src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java > 1132865 > /src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java > 1132865 > /src/java/test/org/apache/zookeeper/test/ClientBase.java 1132865 > /src/java/test/org/apache/zookeeper/test/ReadOnlyModeTest.java 1132865 > /src/zookeeper.jute 1132865 > > Diff: https://reviews.apache.org/r/873/diff > > > Testing > ------- > > > Thanks, > > fpj > >
