[PR] CompactableImpl sync fix and message update [accumulo]

["EdColeman (via GitHub)" <gi...@apache.org>]

EdColeman opened a new pull request, #4198:
URL: https://github.com/apache/accumulo/pull/4198

   Looking at issue #4190 - noticed there is an unnecessary sync in the close method.
   
   Also, added text to the error messages reported in the issue - the same message looks like it can occur from two places, so this may help identity where the issue is occurring.  This does not fix the issue.


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

To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] CompactableImpl sync fix and message update [accumulo]

["EdColeman (via GitHub)" <gi...@apache.org>]

EdColeman merged PR #4198:
URL: https://github.com/apache/accumulo/pull/4198


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

To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] CompactableImpl sync fix and message update [accumulo]

["keith-turner (via GitHub)" <gi...@apache.org>]

keith-turner commented on code in PR #4198:
URL: https://github.com/apache/accumulo/pull/4198#discussion_r1467733031


##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactableImpl.java:
##########
@@ -1557,30 +1558,27 @@ public boolean isMajorCompactionQueued() {
    * should be running and none should be able to start.
    */
   public synchronized void close() {
-    synchronized (this) {

Review Comment:
   Looked into the way this code works, think we should keep this sync block and remove the sync keyword on the method.  There is no need for the last stmt to be in the sync block, the code in `manager.compactableClosed(` is thread safe and its already called by multiple threads because the lock in this code is per tablet and many tablets can already concurrently call `manager.compactableClosed(`. 
   
   



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

To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org