[GitHub] [accumulo] belugabehr commented on issue #1358: Fix race conditions with commitsInProgress counter
belugabehr commented on issue #1358: Fix race conditions with commitsInProgress counter URL: https://github.com/apache/accumulo/pull/1358#issuecomment-531040596 For this particular PR it's still not clear to me why `CommitSession.java` needs to lock (block) the entire tablet to keep this internal count. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [accumulo] belugabehr commented on issue #1358: Fix race conditions with commitsInProgress counter
belugabehr commented on issue #1358: Fix race conditions with commitsInProgress counter URL: https://github.com/apache/accumulo/pull/1358#issuecomment-530944412 You can check out my work on the matter here: https://github.com/belugabehr/accumulo/tree/TabletLock 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [accumulo] belugabehr commented on issue #1358: Fix race conditions with commitsInProgress counter
belugabehr commented on issue #1358: Fix race conditions with commitsInProgress counter URL: https://github.com/apache/accumulo/pull/1358#issuecomment-530936898 Also, typically, I use `isHeldByCurrentThread()` in `private` scoped methods to ensure the lock is not lost with a future change. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [accumulo] belugabehr commented on issue #1358: Fix race conditions with commitsInProgress counter
belugabehr commented on issue #1358: Fix race conditions with commitsInProgress counter URL: https://github.com/apache/accumulo/pull/1358#issuecomment-530933558 @keith-turner Thanks for the input. So, the downside with your proposed solution is that the locking becomes very granular. That is to say, if a method needs to be called with the lock in hand, then any initialization of the method happens withing the context of the lock. If the lock is internal to each class, locks can be acquired as-needed. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [accumulo] belugabehr commented on issue #1358: Fix race conditions with commitsInProgress counter
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: us...@infra.apache.org With regards, Apache Git Services
