> On March 27, 2016, 2:42 p.m., fpj wrote: > > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, > > line 96 > > <https://reviews.apache.org/r/25160/diff/28/?file=1315548#file1315548line96> > > > > Could you align the lines of the comment pls?
Done. > On March 27, 2016, 2:42 p.m., fpj wrote: > > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, > > line 85 > > <https://reviews.apache.org/r/25160/diff/28/?file=1315548#file1315548line85> > > > > Why is this not final any longer? Because the test assigns a mock queue to this variable. See "processor.queuedRequests = new MockRequestsQueue();" in CommitProcessorConcurrencyTest.java > On March 27, 2016, 2:42 p.m., fpj wrote: > > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, > > line 165 > > <https://reviews.apache.org/r/25160/diff/28/?file=1315548#file1315548line165> > > > > These comment lines are too long, please respect the 80 character limit. Done > On March 27, 2016, 2:42 p.m., fpj wrote: > > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, > > line 172 > > <https://reviews.apache.org/r/25160/diff/28/?file=1315548#file1315548line172> > > > > More long lines... Done > On March 27, 2016, 2:42 p.m., fpj wrote: > > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, > > line 190 > > <https://reviews.apache.org/r/25160/diff/28/?file=1315548#file1315548line190> > > > > Can we maintain the comment style /* */, pls? Done > On March 27, 2016, 2:42 p.m., fpj wrote: > > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, > > line 201 > > <https://reviews.apache.org/r/25160/diff/28/?file=1315548#file1315548line201> > > > > You can call just pendingRequests.put(request.sessionId, new > > LinkedList<Request>()); instead and remove the previous line. requests is updated since it is used below this code. > On March 27, 2016, 2:42 p.m., fpj wrote: > > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, > > line 223 > > <https://reviews.apache.org/r/25160/diff/28/?file=1315548#file1315548line223> > > > > Why do we have to wait for empty pool both here and line 245? This is the same as before the patch (check out the condition for waking up the thread right now). Due to synchronization issues in FinalRequestProcessor (potential races between setting watches and updating data), commitRequestProcessor has always either sent it many reads or a single write. We kept this in the patch. I think this part can be made more efficient, e.g., using read/write locks, and I plan to open a separate Jira for this. > On March 27, 2016, 2:42 p.m., fpj wrote: > > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, > > line 405 > > <https://reviews.apache.org/r/25160/diff/28/?file=1315548#file1315548line405> > > > > 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. https://goo.gl/m1cINJ gives more details, please see pseudocode on the last page. The idea is that synchronization on new incoming requests is now separate from synchronization on "thread pool is available to process more requests", since we may have work to do even if there is no "new" incoming requests. > On March 27, 2016, 2:42 p.m., fpj wrote: > > ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java, > > line 151 > > <https://reviews.apache.org/r/25160/diff/28/?file=1315549#file1315549line151> > > > > Long line. Done > On March 27, 2016, 2:42 p.m., fpj wrote: > > ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java, > > line 166 > > <https://reviews.apache.org/r/25160/diff/28/?file=1315549#file1315549line166> > > > > Long line. Done - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25160/#review125565 ----------------------------------------------------------- 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 > >
