keith-turner commented on code in PR #4254:
URL: https://github.com/apache/accumulo/pull/4254#discussion_r1500021001
##########
server/base/src/main/java/org/apache/accumulo/server/metadata/ConditionalTabletMutatorImpl.java:
##########
@@ -231,6 +232,13 @@ private void requireSameSingle(TabletMetadata
tabletMetadata, ColumnType type) {
mutation.addCondition(c);
}
break;
+ case USER_COMPACTION_REQUESTED: {
Review Comment:
Could add test code to AmpleConditionalWriterIT for this change.
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/compact/CompactionDriver.java:
##########
@@ -257,8 +259,14 @@ public int updateAndCheckTablets(Manager manager, FateId
fateId)
otherSelected++;
}
} else {
Review Comment:
Would be good to make the requirements for entering the else explicit rather
than relying on all of the other elseifs
```suggestion
} else if(!tablet.getExternalCompactions().isEmpty() &&
!tablet.getUserCompactionsRequested().contains(fateId)) {
```
##########
test/src/main/java/org/apache/accumulo/test/functional/CompactionIT.java:
##########
@@ -909,6 +912,124 @@ public void testDeleteCompactionService() throws
Exception {
}
}
+ @Test
+ public void testUserCompactionRequested() throws Exception {
+
+ String tableName = getUniqueNames(1)[0];
+ try (final AccumuloClient client =
Accumulo.newClient().from(getClientProps()).build()) {
+
+ // configure tablet compaction iterator that slows compaction down so we
can test
+ // that the USER_COMPACTION_REQUESTED column is set when a user
compaction is requested
+ // when a system compaction is running and blocking
+
+ var ntc = new NewTableConfiguration();
+ IteratorSetting iterSetting = new IteratorSetting(50,
SlowIterator.class);
+ SlowIterator.setSleepTime(iterSetting, 1);
+ ntc.attachIterator(iterSetting, EnumSet.of(IteratorScope.majc));
+ ntc.setProperties(Map.of(Property.TABLE_MAJC_RATIO.getKey(), "20"));
+ client.tableOperations().create(tableName, ntc);
+
+ // Insert MAX_DATA rows
+ writeRows((ClientContext) client, tableName, MAX_DATA, false);
+
+ // set the compaction ratio 1 to trigger a system compaction
+ client.tableOperations().setProperty(tableName,
Property.TABLE_MAJC_RATIO.getKey(), "1");
+
+ var tableId =
TableId.of(client.tableOperations().tableIdMap().get(tableName));
+ var extent = new KeyExtent(tableId, null, null);
+
+ AtomicReference<ExternalCompactionId> initialCompaction = new
AtomicReference<>();
+
+ // Wait for the system compaction to start
+ Wait.waitFor(() -> {
+ var tabletMeta = ((ClientContext)
client).getAmple().readTablet(extent);
+ var externalCompactions = tabletMeta.getExternalCompactions();
+ log.debug("Current external compactions {}",
externalCompactions.size());
+ var current = externalCompactions.keySet().stream().findFirst();
+ current.ifPresent(initialCompaction::set);
+ return current.isPresent();
+ }, Wait.MAX_WAIT_MILLIS, 100);
+
+ // Trigger a user compaction which should be blocked by the system
compaction
+ // and should result in the userRequestedCompactions column being set so
no more
+ // system compactions run
+ client.tableOperations().compact(tableName, new
CompactionConfig().setWait(false));
+ Wait.waitFor(() -> {
+ var tabletMeta = ((ClientContext)
client).getAmple().readTablet(extent);
+ var userRequestedCompactions =
tabletMeta.getUserCompactionsRequested().size();
+ log.debug("Current user requested compaction markers {}",
userRequestedCompactions);
+ return userRequestedCompactions == 1;
+ }, Wait.MAX_WAIT_MILLIS, 100);
+
+ // Send more data to trigger another system compaction but the user
compaction
+ // should go next and the column marker should block it
+ writeRows((ClientContext) client, tableName, MAX_DATA, false);
+
+ // Verify that when the next compaction starts it is a USER compaction
as the
+ // SYSTEM compaction should be blocked by the marker
+ Wait.waitFor(() -> {
+ var tabletMeta = ((ClientContext)
client).getAmple().readTablet(extent);
+ log.debug("Waiting for USER compaction to start {}", extent);
+
+ var userRequestedCompactions =
tabletMeta.getUserCompactionsRequested().size();
+ log.debug("Current user requested compaction markers {}",
userRequestedCompactions);
+ var externalCompactions = tabletMeta.getExternalCompactions();
+ log.debug("External compactions size {}", externalCompactions.size());
+ var current = externalCompactions.entrySet().stream().findFirst();
+ current.ifPresent(c -> log.debug("Current running compaction {}",
c.getKey()));
+
+ if (current.isPresent()) {
+ var currentCompaction = current.orElseThrow();
+ log.debug("Current running compaction {}",
currentCompaction.getKey());
Review Comment:
I think this log stmt is redundant.
##########
server/base/src/main/java/org/apache/accumulo/server/constraints/MetadataConstraints.java:
##########
@@ -266,7 +269,8 @@ public List<Short> check(Environment env, Mutation
mutation) {
} catch (RuntimeException e) {
violations = addViolation(violations, 11);
}
- } else if (CompactedColumnFamily.NAME.equals(columnFamily)) {
+ } else if (CompactedColumnFamily.NAME.equals(columnFamily)
+ || UserCompactionRequestedColumnFamily.NAME.equals(columnFamily)) {
Review Comment:
Could add test to MetadataConstraintsTest for this change
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/compact/CompactionDriver.java:
##########
@@ -257,8 +259,14 @@ public int updateAndCheckTablets(Manager manager, FateId
fateId)
otherSelected++;
}
} else {
- // ELASTICITY_TODO if there are compactions preventing selection of
files, then add
+ // If there are compactions preventing selection of files, then add
// selecting marker that prevents new compactions from starting
+ log.debug("Marking {} as needing a user requested compaction for
{}", tablet.getExtent(),
+ fateId);
+ var mutator =
tabletsMutator.mutateTablet(tablet.getExtent()).requireAbsentOperation()
+ .requireSame(tablet, ECOMP, USER_COMPACTION_REQUESTED)
+ .putUserCompactionRequested(fateId);
+ mutator.submit(tm ->
tm.getUserCompactionsRequested().contains(fateId));
otherCompaction++;
Review Comment:
We could break this `otherCompacion`counter into two counters. One to
indicate we set the new column and another to indicate the new colwas
previously set and now we are just waiting. The counters are only used in the
debug stmt,I have found them useful to track what exactly is happening when
something goes wrong with compactions.
--
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]
