----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43957/#review120607 -----------------------------------------------------------
core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchWriter.java <https://reviews.apache.org/r/43957/#comment182080> This notification may be important. W/o it other threads may wait longer than needed or possibly forever core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchWriter.java (line 284) <https://reviews.apache.org/r/43957/#comment182073> Without sync I think the following can happen, which will result in a lost mutation. * Thread 1 calls mutations.get() * Thread 2 bins/processes the mutation set that thread 1 has a local reference to * Thread 1 adds a mutation to a dead mutation set that is already processed/binned. core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchWriter.java (line 973) <https://reviews.apache.org/r/43957/#comment182074> race conditions here w/o enclosing sync I pointed out some concurrency issues and I think there are more. HAving many volatile makes it very hard to reason about things. Between reading two volatiles, other threads can make changes to the system state. Instead of making these changes with volatiles, we could possibly make the following change. ```java class TabletServerBatchWriter { private class MutationWriter { void addMutations(MutationSet mutationsToSend) { //put task on thread pool to do what the code in this method is currently doing... } } } ``` - kturner On Feb. 24, 2016, 11:10 p.m., Dave Marion wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43957/ > ----------------------------------------------------------- > > (Updated Feb. 24, 2016, 11:10 p.m.) > > > Review request for accumulo and Josh Elser. > > > Bugs: ACCUMULO-1755 > https://issues.apache.org/jira/browse/ACCUMULO-1755 > > > Repository: accumulo > > > Description > ------- > > ACCUMULO-1755: removed synchronized modifier from > TabletServerBatchWriterstartProcessing() > > > Diffs > ----- > > > core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchWriter.java > bc90d00 > > Diff: https://reviews.apache.org/r/43957/diff/ > > > Testing > ------- > > unit tests in core pass > > > Thanks, > > Dave Marion > >
