----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25160/#review119463 -----------------------------------------------------------
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 158) <https://reviews.apache.org/r/25160/#comment180829> Please add a comment saying that Since requests are placed in the queue before being sent to the leader, if commitIsWaiting = true, the commit belongs to one of the first requestsToProcess operations in the queue or to a request from a client on another server (order of the following two lines is important!). Add that in each iteration we process up to requestsToProcess requests from the incoming queue (possibly less if a committed request can be matched to a pending local write) and one committed request if commitIsWaiting. ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 184) <https://reviews.apache.org/r/25160/#comment180830> add that this is because committs for local requests arrive in the order they appeared in the queue, so if we have something in pending and a committed request, the committed request must be for that pending write or for a write originating at a different server ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java (line 229) <https://reviews.apache.org/r/25160/#comment180835> could you add several commits ? with just one its possible that we're just processing it first ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java (line 287) <https://reviews.apache.org/r/25160/#comment180832> there are 100 requests in this loop already. Do we need the previous single commit ? if so please update the test plan to say 101 commits ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java (line 301) <https://reviews.apache.org/r/25160/#comment180834> Can you invoke twice without the loop and check state after each invocation ? Can you check that the rest of the committed requests remain in the committed queue and are not in the processedRequests - Alexander Shraer On Feb. 17, 2016, 2:11 p.m., Kfir Lev-Ari wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25160/ > ----------------------------------------------------------- > > (Updated Feb. 17, 2016, 2:11 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 > >
