[GitHub] [accumulo] belugabehr commented on issue #1358: Fix race conditions with commitsInProgress counter

[GitBox <gi...@apache.org>]

belugabehr commented on issue #1358: Fix race conditions with commitsInProgress counter
URL: https://github.com/apache/accumulo/pull/1358#issuecomment-530824196
 
 
   @ctubbsii Yes. Tablet appears to be a 'hot' lock.  I am actually looking at that class but it's a bit of a beast.  Anyway we can remove these external `Tablet` locks will make it easier later.
   
   This class is a good candidate to improve:
   
   1. I believe the lock on `Tablet` is superfluous
   2. Update to use Java Concurrency classes (as an example for the rest of the code base)
   3. I do not trust this code because `commitsInProgress` is accessed by different threads but is not `volatile`.  In particular, it may be the case that `waitForCommitsToFinish` loops forever because the thread has cached the value of `commitsInProgress`
   4. `decrementCommitsInProgress` and `incrementCommitsInProgress` are not synchronized so incrementing and decrementing `commitsInProgress` seems suspect
   
   I say that the lock on `Tablet` is superfluous here because the `Tablet` is being `notifyAll()` when `commitsInProgress` is zero.  The thing is that the only code that cares about this `notifyAll()` is in `waitForCommitsToFinish`.  The `commitsInProgress` value is private to this class and only accessed in the local method `waitForCommitsToFinish`.  So, all the threads locking on `Tablet` will be awoken here, but none of them have any way to respond to this event, except for `waitForCommitsToFinish`, so why bother waking up every thread?  Just wake up the threads waiting on `commitsInProgress` to be zero.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services