----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25160/#review125565 -----------------------------------------------------------
The patch looks mostly good. I have several trivial comments, but some around synchronization that I want to understand better. I haven't looked at the tests, though, but I'll leave it for later. ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 84) <https://reviews.apache.org/r/25160/#comment188437> Why is this not final any longer? ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 95) <https://reviews.apache.org/r/25160/#comment188438> Could you align the lines of the comment pls? ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 155) <https://reviews.apache.org/r/25160/#comment188432> These comment lines are too long, please respect the 80 character limit. ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 162) <https://reviews.apache.org/r/25160/#comment188439> More long lines... ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 177) <https://reviews.apache.org/r/25160/#comment188433> Can we maintain the comment style /* */, pls? ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 188) <https://reviews.apache.org/r/25160/#comment188436> You can call just pendingRequests.put(request.sessionId, new LinkedList<Request>()); instead and remove the previous line. ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 210) <https://reviews.apache.org/r/25160/#comment188441> Why do we have to wait for empty pool both here and line 245? ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 337) <https://reviews.apache.org/r/25160/#comment188440> This wakeupOnEmpty() call is synchronizing on a different object compared to wakeup(). I'm not entirely clear on how the synchronization goes here because it looks like we are now synchronizing on both this and emptyPoolSync. ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java (line 150) <https://reviews.apache.org/r/25160/#comment188435> Long line. ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java (line 165) <https://reviews.apache.org/r/25160/#comment188434> Long line. - fpj On March 26, 2016, 7:06 p.m., Kfir Lev-Ari wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25160/ > ----------------------------------------------------------- > > (Updated March 26, 2016, 7:06 p.m.) > > > Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer. > > > Repository: zookeeper > > > Description > ------- > > Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024 > > > Diffs > ----- > > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java > 1726708 > > ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java > 1726708 > ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java > 1726708 > > Diff: https://reviews.apache.org/r/25160/diff/ > > > Testing > ------- > > The attached unit tests, as well as the system test found in > https://issues.apache.org/jira/browse/ZOOKEEPER-2023. > > > Thanks, > > Kfir Lev-Ari > >
