accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ke...@deenlo.com
Subject Re: Review Request 43957: ACCUMULO-1755: removed synchronized modifier from TabletServerBatchWriterstartProcessing()
Date Thu, 25 Feb 2016 00:12:07 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message