ctubbsii commented on a change in pull request #2213:
URL: https://github.com/apache/accumulo/pull/2213#discussion_r678580735
##########
File path:
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactableImpl.java
##########
@@ -214,6 +202,9 @@ public CompactableImpl(Tablet tablet, CompactionManager
manager,
}
return Set.copyOf(servicesIds);
}, 2, TimeUnit.SECONDS);
+
+ this.fileMgr = new
TabletCompactionFileManager(Collections.unmodifiableSet(runningJobs),
+ tablet.getExtent(), extCompactingFiles, extSelInfo);
Review comment:
I think the inner class makes sense. However, it's still hard to verify
that all the accesses are synchronized. For example, `noneRunning` has a read
on `runningJobs` but is not synchronized. Tracing it back, it is called by some
other methods which are also not synchronized... but then those methods are
called by other methods which are called only in synchronized blocks. Requiring
all these uses to be traced back to verify the concurrency model is correct is
frustrating and error-prone. I wish there was a better way, but not sure of
what could work better. Maybe you could throw in an extra synchronized keyword
or two to guard `runningJobs`, so it's less error-prone, taking advantage of
reentrancy so the redundant synchronized keywords don't matter. Either way, I
think the tight coupling is more clear as an inner-class. Thanks for making
that 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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
