keith-turner commented on code in PR #4480:
URL: https://github.com/apache/accumulo/pull/4480#discussion_r1605548755
##########
test/src/main/java/org/apache/accumulo/test/functional/CompactionIT.java:
##########
@@ -1011,6 +1040,117 @@ public void testUserCompactionRequested() throws
Exception {
ExternalCompactionTestUtils.assertNoCompactionMetadata(getServerContext(),
tableName);
}
+ @Test
+ public void testCancelUserCompaction() throws Exception {
Review Comment:
This is a nice test w/ good comments.
##########
server/manager/src/main/java/org/apache/accumulo/manager/compaction/coordinator/CompactionCoordinator.java:
##########
@@ -561,6 +566,20 @@ protected CompactionMetadata
reserveCompaction(CompactionJobQueues.MetaJob metaJ
var tabletMutator =
tabletsMutator.mutateTablet(extent).requireAbsentOperation()
.requireSame(tabletMetadata, FILES, SELECTED, ECOMP);
+ if (metaJob.getJob().getKind() == CompactionKind.SYSTEM) {
+ var selectedFiles = tabletMetadata.getSelectedFiles();
+ var reserved = getFilesReservedBySelection(tabletMetadata,
manager.getSteadyTime(), ctx);
+
+ // If there is a selectedFiles column, and the reserved set is empty
this means that
+ // either no user jobs were completed yet or the selection
expiration time has passed
+ // so the column is eligible to be deleted so a system job can run
instead
+ if (selectedFiles != null && reserved.isEmpty()
+ && !Collections.disjoint(jobFiles, selectedFiles.getFiles())) {
+ LOG.debug("Deleting user compaction selected files for {}",
extent);
Review Comment:
```suggestion
LOG.debug("Deleting user compaction selected files for {} {}",
extent, externalCompactionId);
```
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/compact/CompactionDriver.java:
##########
@@ -241,17 +242,27 @@ public int updateAndCheckTablets(Manager manager, FateId
fateId)
noneSelected++;
} else {
var mutator =
tabletsMutator.mutateTablet(tablet.getExtent()).requireAbsentOperation()
- .requireSame(tablet, FILES, SELECTED, ECOMP, COMPACTED);
- var selectedFiles =
- new SelectedFiles(filesToCompact,
tablet.getFiles().equals(filesToCompact), fateId);
+ .requireSame(tablet, FILES, SELECTED, ECOMP, COMPACTED,
USER_COMPACTION_REQUESTED);
+ var selectedFiles = new SelectedFiles(filesToCompact,
+ tablet.getFiles().equals(filesToCompact), fateId, time);
mutator.putSelectedFiles(selectedFiles);
+ // We no longer need to include this marker if files are selected
+ if (tablet.getUserCompactionsRequested().contains(fateId)) {
+ mutator.deleteUserCompactionRequested(fateId);
+ }
+
selectionsSubmitted.put(tablet.getExtent(), filesToCompact);
+ // TODO: Do we need to handle a race condition for the rejection
handler check
Review Comment:
Re this comment. Could relax the check in the rejection handler to
`tabletMetadata.getSelectedFiles().getFateId().equals(fateId) ||
tabletMetadata.getCompacted().contains(fateId)` if either of those are true
then write when through, but could have changed since to compact some or even
completed.
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/compact/CompactionDriver.java:
##########
@@ -186,6 +186,7 @@ public int updateAndCheckTablets(Manager manager, FateId
fateId)
var tabletsMutator = ample.conditionallyMutateTablets(resultConsumer))
{
CompactionConfig config =
CompactionConfigStorage.getConfig(manager.getContext(), fateId);
+ var time = manager.getSteadyTime();
Review Comment:
May be better to grab the steady time each time its needed. Thinking for a
large amount of tablets it could take a while to scan and write out data and
this may get stale. Seems like it ok to have this be different for different
tablets. Looking at the impl of manager.getSteadyTime(), it seems like it
should be quick to call.
--
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]
