keith-turner commented on a change in pull request #2263:
URL: https://github.com/apache/accumulo/pull/2263#discussion_r704739058
##########
File path:
server/tserver/src/main/java/org/apache/accumulo/tserver/compactions/CompactionService.java
##########
@@ -223,15 +224,22 @@ private boolean reconcile(Set<CompactionJob> jobs,
Collection<SubmittedJob> subm
return true;
}
- public void compact(CompactionKind kind, Compactable compactable,
+ /**
+ * Get compaction plan for the provided compactable tablet and possibly
submit for compaction.
+ * Plans get added to the planning queue before calling the planningExecutor
to get the plan. If
+ * no files are selected, return. Otherwise, submit the compaction job.
+ */
+ public void submitCompaction(CompactionKind kind, Compactable compactable,
Consumer<Compactable> completionCallback) {
Objects.requireNonNull(compactable);
+ // add tablet to planning queue and use planningExecutor to get the plan
if (queuedForPlanning.get(kind).putIfAbsent(compactable.getExtent(),
compactable) == null) {
try {
planningExecutor.execute(() -> {
try {
- planCompaction(kind, compactable, completionCallback);
+ Optional<CompactionPlan> plan = getCompactionPlan(kind,
compactable);
+ plan.ifPresent(cp -> submitCompactionJob(cp, compactable,
completionCallback));
Review comment:
As mentioned in other comments, should probably not add candidates to
plan and the reason it was added does not actually log all of the info. Think
the following changes may get the needed information where it needs to go w/o
modifying the plan interface.
```suggestion
var files = compactable.getFiles(...);
Optional<CompactionPlan> plan = getCompactionPlan(kind, files,
compactable.getExtent());
plan.ifPresent(cp -> submitCompactionJob(cp, files,
compactable.getExtent(), completionCallback));
```
##########
File path:
core/src/main/java/org/apache/accumulo/core/spi/compaction/CompactionPlan.java
##########
@@ -55,5 +56,13 @@ Builder addJob(short priority, CompactionExecutorId executor,
CompactionPlan build();
}
+ /**
+ * Return the set of compaction candidates.
+ */
+ Set<CompactableFile> getCandidates();
Review comment:
Thinking this should not be added to SPI. It has nothing to do with the
plan for a compaction, but is a subset of the information that went into
creating a plan. Also I suggested other changes that would not require this
change.
##########
File path:
server/tserver/src/main/java/org/apache/accumulo/tserver/compactions/CompactionService.java
##########
@@ -363,11 +374,11 @@ private void planCompaction(CompactionKind kind,
Compactable compactable,
if (!jobs.isEmpty()) {
log.trace("Submitted compaction plan {} id:{} files:{} plan:{}",
compactable.getExtent(),
- myId, files, plan);
+ myId, plan.getCandidates(), plan);
Review comment:
With changes in other comment could revert to the following. Also this
changes is logging less information than it used to. Used to log the
Compactable.Files obj now it logging a subset of that info.
```suggestion
myId, files, plan);
```
--
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]
