keith-turner commented on a change in pull request #2213:
URL: https://github.com/apache/accumulo/pull/2213#discussion_r676944262
##########
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:
The classes are meant to be tightly coupled. It was never intended that
anything besides CompactableImpl would use TabletCompactionFileManager. Making
it an inner class seems like a good way to express this. Tracking the state of
compacting files and which files are eligible for compaction is one of the most
complicated parts of CompactableImpl currently. The goal with this change was
to logically group that code to make it easier to test and modify. However I
don't want to attempt to make it a standalone unit in terms of functionality or
synchronization as I think doing this would make the code harder to understand
and verify for correctness. I can not really think of anything simpler to
understand than the current synchronized code blocks in CompactableImpl.
Making it an inner class will make it clear that is tightly coupled while still
allowing testing and logical grouping of the code that does bookkeeping for
compacting files.
--
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]
