You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by fp...@apache.org on 2011/06/09 16:39:27 UTC
Review Request: ZOOKEEPER-335.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/873/
-----------------------------------------------------------
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
Re: Review Request: ZOOKEEPER-335.
Posted by Benjamin Reed <br...@yahoo-inc.com>.
> On 2011-06-09 16:00:28, fpj wrote:
> > /src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java, line 167
> > <https://reviews.apache.org/r/873/diff/1/?file=20836#file20836line167>
> >
> > This comment is incorrect, it should be peer epoch.
i think it should be the epoch of the proposed leader correct?
> On 2011-06-09 16:00:28, fpj wrote:
> > /src/java/main/org/apache/zookeeper/server/quorum/Leader.java, line 805
> > <https://reviews.apache.org/r/873/diff/1/?file=20838#file20838line805>
> >
> > 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.
yes, should i comment that? it is used often in LearnerHandler.
> On 2011-06-09 16:00:28, fpj wrote:
> > /src/java/test/org/apache/zookeeper/test/ReadOnlyModeTest.java, line 202
> > <https://reviews.apache.org/r/873/diff/1/?file=20849#file20849line202>
> >
> > 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.
that line is being added. the readonlymode test counts on the servers to come out of readonly mode at that point. without the wait we have a race. (a race that the test kept losing on my computer.)
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/873/#review790
-----------------------------------------------------------
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
>
>
Re: Review Request: ZOOKEEPER-335.
Posted by fp...@apache.org.
-----------------------------------------------------------
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
>
>