----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/483/#review407 -----------------------------------------------------------
Hi Abmar - This is a great (big) patch - looks really good. I've done a very quick pass over the first page and a half of the code for stylistic concerns, and a few comments about small structural details. Please extrapolate from what I've said - formatting nits, javadoc on public APIs, cut down on code duplication, fewer unchecked exceptions (IMO) - where you agree. Thanks! Henry trunk/src/java/main/org/apache/zookeeper/ClientCnxn.java <https://reviews.apache.org/r/483/#comment778> nit: spaces around / trunk/src/java/main/org/apache/zookeeper/ClientCnxn.java <https://reviews.apache.org/r/483/#comment779> nit: space after , trunk/src/java/main/org/apache/zookeeper/ClientCnxn.java <https://reviews.apache.org/r/483/#comment780> Please comment this enum trunk/src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java <https://reviews.apache.org/r/483/#comment781> ResponseType is an enum, so I think a switch statement would be preferred for this block. trunk/src/java/main/org/apache/zookeeper/ZooKeeper.java <https://reviews.apache.org/r/483/#comment782> Would add a comment to the effect that the default failure detector will be used here. trunk/src/java/main/org/apache/zookeeper/ZooKeeperMain.java <https://reviews.apache.org/r/483/#comment783> I know we're poor at doing this elsewhere in the code, but please add javadoc to public APIs trunk/src/java/main/org/apache/zookeeper/ZooKeeperMain.java <https://reviews.apache.org/r/483/#comment784> It's a small nit, but why can't the FDFactory tell us what the available implementations are? trunk/src/java/main/org/apache/zookeeper/common/fd/BertierFailureDetector.java <https://reviews.apache.org/r/483/#comment787> By not forcing the caller to check for this exception, you're allowing this call to silently fail and blow up the current thread. A bit scary for a long-lived distributed system. Suggest either silently *succeeding* (and mentioning this in the javadoc) or throwing a checked exception, or returning a success code. trunk/src/java/main/org/apache/zookeeper/common/fd/BertierFailureDetector.java <https://reviews.apache.org/r/483/#comment786> IllegalArgumentException is an unchecked exception, and I'm not sure it's the right thing to use here. Why not just have the caller test for null? trunk/src/java/main/org/apache/zookeeper/common/fd/BertierFailureDetector.java <https://reviews.apache.org/r/483/#comment785> Nit: space after //, for all of these trunk/src/java/main/org/apache/zookeeper/common/fd/FixedPingFailureDetector.java <https://reviews.apache.org/r/483/#comment788> You're duplicating this code in at least two places. Could you either inherit from an abstract base class, or, through composition, delegate to a 'monitor store' or something? Again, the possible silent exception in every public API of this class is a concern. trunk/src/java/main/org/apache/zookeeper/common/fd/InterArrivalSamplingWindow.java <https://reviews.apache.org/r/483/#comment789> Make these two final? trunk/src/java/main/org/apache/zookeeper/common/fd/InterArrivalSamplingWindow.java <https://reviews.apache.org/r/483/#comment790> Nit: spacing around operators is inconsistent trunk/src/java/main/org/apache/zookeeper/server/quorum/LearnerSessionTracker.java <https://reviews.apache.org/r/483/#comment791> Single character variable names are a bit inscrutable :) trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java <https://reviews.apache.org/r/483/#comment792> Extra blank line - Henry On 2011-04-09 16:20:30, Camille Fournier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/483/ > ----------------------------------------------------------- > > (Updated 2011-04-09 16:20:30) > > > Review request for zookeeper. > > > Summary > ------- > > https://issues.apache.org/jira/browse/ZOOKEEPER-702 > > > Diffs > ----- > > trunk/src/docs/src/documentation/content/xdocs/index.xml 1065709 > trunk/src/docs/src/documentation/content/xdocs/zookeeperFailureDetector.xml > PRE-CREATION > trunk/src/java/main/org/apache/zookeeper/ClientCnxn.java 1065709 > trunk/src/java/main/org/apache/zookeeper/ClientCnxnSocket.java 1065709 > trunk/src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 1065709 > trunk/src/java/main/org/apache/zookeeper/ZooKeeper.java 1065709 > trunk/src/java/main/org/apache/zookeeper/ZooKeeperMain.java 1065709 > > trunk/src/java/main/org/apache/zookeeper/common/fd/BertierFailureDetector.java > PRE-CREATION > trunk/src/java/main/org/apache/zookeeper/common/fd/ChenFailureDetector.java > PRE-CREATION > trunk/src/java/main/org/apache/zookeeper/common/fd/FailureDetector.java > PRE-CREATION > > trunk/src/java/main/org/apache/zookeeper/common/fd/FailureDetectorFactory.java > PRE-CREATION > > trunk/src/java/main/org/apache/zookeeper/common/fd/FailureDetectorOptParser.java > PRE-CREATION > > trunk/src/java/main/org/apache/zookeeper/common/fd/FixedPingFailureDetector.java > PRE-CREATION > > trunk/src/java/main/org/apache/zookeeper/common/fd/InterArrivalSamplingWindow.java > PRE-CREATION > trunk/src/java/main/org/apache/zookeeper/common/fd/MessageType.java > PRE-CREATION > > trunk/src/java/main/org/apache/zookeeper/common/fd/PhiAccrualFailureDetector.java > PRE-CREATION > > trunk/src/java/main/org/apache/zookeeper/common/fd/SlicedPingFailureDetector.java > PRE-CREATION > trunk/src/java/main/org/apache/zookeeper/server/ServerConfig.java 1065709 > trunk/src/java/main/org/apache/zookeeper/server/SessionTracker.java 1065709 > trunk/src/java/main/org/apache/zookeeper/server/SessionTrackerImpl.java > 1065709 > trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java > 1065709 > trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java > 1065709 > > trunk/src/java/main/org/apache/zookeeper/server/quorum/FollowerZooKeeperServer.java > 1065709 > trunk/src/java/main/org/apache/zookeeper/server/quorum/Leader.java 1065709 > > trunk/src/java/main/org/apache/zookeeper/server/quorum/LeaderZooKeeperServer.java > 1065709 > trunk/src/java/main/org/apache/zookeeper/server/quorum/Learner.java 1072085 > trunk/src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java > 1065709 > > trunk/src/java/main/org/apache/zookeeper/server/quorum/LearnerSessionTracker.java > 1065709 > > trunk/src/java/main/org/apache/zookeeper/server/quorum/LearnerZooKeeperServer.java > 1065709 > > trunk/src/java/main/org/apache/zookeeper/server/quorum/ObserverZooKeeperServer.java > 1065709 > trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java > 1089594 > > trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java > 1089595 > trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java > 1065709 > > trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumZooKeeperServer.java > 1065709 > trunk/src/java/test/org/apache/zookeeper/TestableZooKeeper.java 1065709 > trunk/src/java/test/org/apache/zookeeper/test/ClientBase.java 1065709 > trunk/src/java/test/org/apache/zookeeper/test/DisconnectableZooKeeper.java > 1065709 > trunk/src/java/test/org/apache/zookeeper/test/QuorumBase.java 1065709 > trunk/src/java/test/org/apache/zookeeper/test/QuorumFDHammerTest.java > PRE-CREATION > trunk/src/java/test/org/apache/zookeeper/test/RecoveryTest.java 1065709 > trunk/src/java/test/org/apache/zookeeper/test/SessionTest.java 1065709 > > trunk/src/java/test/org/apache/zookeeper/test/fd/BertierClientHammerTest.java > PRE-CREATION > trunk/src/java/test/org/apache/zookeeper/test/fd/BertierFDTest.java > PRE-CREATION > > trunk/src/java/test/org/apache/zookeeper/test/fd/BertierQuorumHammerTest.java > PRE-CREATION > trunk/src/java/test/org/apache/zookeeper/test/fd/BertierRecoveryTest.java > PRE-CREATION > trunk/src/java/test/org/apache/zookeeper/test/fd/BertierSessionTest.java > PRE-CREATION > trunk/src/java/test/org/apache/zookeeper/test/fd/ChenClientHammerTest.java > PRE-CREATION > trunk/src/java/test/org/apache/zookeeper/test/fd/ChenFDTest.java > PRE-CREATION > trunk/src/java/test/org/apache/zookeeper/test/fd/ChenQuorumHammerTest.java > PRE-CREATION > trunk/src/java/test/org/apache/zookeeper/test/fd/ChenRecoveryTest.java > PRE-CREATION > trunk/src/java/test/org/apache/zookeeper/test/fd/ChenSessionTest.java > PRE-CREATION > trunk/src/java/test/org/apache/zookeeper/test/fd/FixedPingFDTest.java > PRE-CREATION > > trunk/src/java/test/org/apache/zookeeper/test/fd/InterArrivalSamplingWindowTest.java > PRE-CREATION > > trunk/src/java/test/org/apache/zookeeper/test/fd/PhiAccrualClientHammerTest.java > PRE-CREATION > trunk/src/java/test/org/apache/zookeeper/test/fd/PhiAccrualFDTest.java > PRE-CREATION > > trunk/src/java/test/org/apache/zookeeper/test/fd/PhiAccrualQuorumHammerTest.java > PRE-CREATION > > trunk/src/java/test/org/apache/zookeeper/test/fd/PhiAccrualRecoveryTest.java > PRE-CREATION > trunk/src/java/test/org/apache/zookeeper/test/fd/PhiAccrualSessionTest.java > PRE-CREATION > trunk/src/java/test/org/apache/zookeeper/test/fd/SlicedPingFDTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/483/diff > > > Testing > ------- > > unit tests > > > Thanks, > > Camille > >
