Review Board 1.7.22


Review Request #873 - Created June 9, 2011 and submitted

breed, fpj, vishalmlst

Posted (June 9, 2011, 4 p.m.)


I'm not sure the read means you have a tab there.
Another red mark.
This comment is incorrect, it should be peer epoch.
  1. i think it should be the epoch of the proposed leader correct?
Another red mark.
More red (this one was here already apparently).
We need to fix it to return the value of the current epoch.
This is the last red I report. There seem to be a number of them, and I'm not sure what they mean.
Formatting seems to be incorrect.
If I understand correctly the semantics of this exception thrown here, it is caught in the main try block of, 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.
  1. yes, should i comment that? it is used often in LearnerHandler.
Small comment: We should probably refer to protocol in the name of the variable, something like leaderProtoVersion.
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...
We probably need to document we have these files that the protocol relies upon for correctness.
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 
Do we want to keep these System.err.println statements? Isn't it better to make them LOG.debug?
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.
  1. 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.)