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: notifications-unsubscr...@accumulo.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org