----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6260/#review9710 -----------------------------------------------------------
This looks very nice Jay! I've looked through most of the code but not yet the CP logic itself. My thoughts so far: afaict the approach seems sound. Needs updates to the documentation. Needs unit tests to verify the new cases. /src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java <https://reviews.apache.org/r/6260/#comment20711> Is this a bug fix? If so it should be separated out to another jira and a test should be added for it. (likely we'd want to fix it in 3.3/3.4/trunk) /src/java/main/org/apache/zookeeper/server/WorkerService.java <https://reviews.apache.org/r/6260/#comment20716> make this configurable. how did you come to 5 seconds as the default? /src/java/main/org/apache/zookeeper/server/WorkerService.java <https://reviews.apache.org/r/6260/#comment20718> doc this can be 0, and any other implications? /src/java/main/org/apache/zookeeper/server/WorkerService.java <https://reviews.apache.org/r/6260/#comment20717> move the method specific docs to the javadoc of the methods themselves. /src/java/main/org/apache/zookeeper/server/WorkerService.java <https://reviews.apache.org/r/6260/#comment20719> seems you need to be a bit careful when calling stop, if schedule() is already past the "stoped" check you could end up with a RejectedExecutionException being thrown? However in this case it seems only CommitProcessor shutdown is calling this.. /src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java <https://reviews.apache.org/r/6260/#comment20714> convert these to javadoc so they show up in eclipse tools tips - Patrick Hunt On July 31, 2012, 10:05 p.m., Jay Shrauner wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6260/ > ----------------------------------------------------------- > > (Updated July 31, 2012, 10:05 p.m.) > > > Review request for zookeeper and Patrick Hunt. > > > Description > ------- > > See https://issues.apache.org/jira/browse/ZOOKEEPER-1505 > > > This addresses bug ZOOKEEPER-1505. > https://issues.apache.org/jira/browse/ZOOKEEPER-1505 > > > Diffs > ----- > > /src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java > 1366784 > /src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java > 1366784 > /src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java 1366784 > /src/java/main/org/apache/zookeeper/server/WorkerService.java PRE-CREATION > /src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java > 1366784 > /src/java/main/org/apache/zookeeper/server/quorum/Leader.java 1366784 > > Diff: https://reviews.apache.org/r/6260/diff/ > > > Testing > ------- > > > Thanks, > > Jay Shrauner > >
