You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "keith-turner (via GitHub)" <gi...@apache.org> on 2025/03/10 21:52:33 UTC
[PR] adds an IT to verify compaction cancels on tablet migration [accumulo]
keith-turner opened a new pull request, #5395:
URL: https://github.com/apache/accumulo/pull/5395
Still have a few TODOs, so opened as a draft.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds an IT to verify compaction cancels on tablet migration [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #5395:
URL: https://github.com/apache/accumulo/pull/5395#issuecomment-2711947628
After running this test, see the following in the logs.
```
$ grep 'canceled' *
TabletServer_1449570859.out:2025-03-10T22:00:19,779 309 [compaction.FileCompactor] DEBUG: Compaction canceled 1;005000;004000
TabletServer_1449570859.out:2025-03-10T22:00:19,779 309 [tablet.CompactableImpl] DEBUG: Compaction canceled 1;005000;004000
TabletServer_1449570859.out:2025-03-10T22:00:22,794 327 [compaction.FileCompactor] DEBUG: Compaction canceled 1;007000;006000
TabletServer_1449570859.out:2025-03-10T22:00:22,794 327 [tablet.CompactableImpl] DEBUG: Compaction canceled 1;007000;006000
TabletServer_1449570859.out:2025-03-10T22:00:25,801 324 [compaction.FileCompactor] DEBUG: Compaction canceled 1;008000;007000
TabletServer_1449570859.out:2025-03-10T22:00:25,801 324 [tablet.CompactableImpl] DEBUG: Compaction canceled 1;008000;007000
TabletServer_941199483.out:2025-03-10T22:00:19,760 324 [compaction.FileCompactor] DEBUG: Compaction canceled 1;015000;014000
TabletServer_941199483.out:2025-03-10T22:00:19,760 324 [tablet.CompactableImpl] DEBUG: Compaction canceled 1;015000;014000
TabletServer_941199483.out:2025-03-10T22:00:28,708 297 [compaction.FileCompactor] DEBUG: Compaction canceled 1;010000;009000
TabletServer_941199483.out:2025-03-10T22:00:28,708 297 [tablet.CompactableImpl] DEBUG: Compaction canceled 1;010000;009000
TabletServer_941199483.out:2025-03-10T22:00:28,767 322 [compaction.FileCompactor] DEBUG: Compaction canceled 1;016000;015000
TabletServer_941199483.out:2025-03-10T22:00:28,768 322 [tablet.CompactableImpl] DEBUG: Compaction canceled 1;016000;015000
```
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] implements interuption of compactions and adds an IT to verify compaction cancels on tablet migration [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5395:
URL: https://github.com/apache/accumulo/pull/5395#discussion_r1991817761
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactableImpl.java:
##########
@@ -1340,8 +1391,11 @@ public void compact(CompactionServiceId service, CompactionJob job, BooleanSuppl
SortedMap<StoredTabletFile,DataFileValue> allFiles = tablet.getDatafiles();
HashMap<StoredTabletFile,DataFileValue> compactFiles = new HashMap<>();
cInfo.jobFiles.forEach(file -> compactFiles.put(file, allFiles.get(file)));
-
- stats = CompactableUtils.compact(tablet, job, cInfo, compactEnv, compactFiles, tmpFileName);
+ // Limit interrupting compactions to the part that reads and writes files and avoid
+ // interrupting the metadata table updates.
+ try (var ignored = new CompactionInterrupter()) {
+ stats = CompactableUtils.compact(tablet, job, cInfo, compactEnv, compactFiles, tmpFileName);
Review Comment:
The main difference is that compactor.interrupt() was not calling Thread.interrupt(). However it could call that, it was already tracking the thread running the compaction. Modified it do that in 4e24af0 moving where Thread.interrupt() is called down a level (this will probably benefit external compactions also). The test still pass w/ this change, however they were a bit slower because the interrupting the thread could take up 10 seconds after calling close(). So changed the scheduled timer to run every 3 seconds. The previous code would immediately interrupt the thread when close() was called.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] implements interuption of compactions and adds an IT to verify compaction cancels on tablet migration [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #5395:
URL: https://github.com/apache/accumulo/pull/5395#issuecomment-2718406437
For the change to isCompactionEnabled() the way the code is working w/ the latest that changes it would probably not help to check for interrupted there. The interrupt stats will be set on the compaction thread when is compaction isCompactionEnabled() returns false. Also another thead besides the compaction thread could call isCompactionEnabled(), so could not use Thread.currentThread()
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] implements interuption of compactions and adds an IT to verify compaction cancels on tablet migration [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5395:
URL: https://github.com/apache/accumulo/pull/5395#discussion_r1991817761
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactableImpl.java:
##########
@@ -1340,8 +1391,11 @@ public void compact(CompactionServiceId service, CompactionJob job, BooleanSuppl
SortedMap<StoredTabletFile,DataFileValue> allFiles = tablet.getDatafiles();
HashMap<StoredTabletFile,DataFileValue> compactFiles = new HashMap<>();
cInfo.jobFiles.forEach(file -> compactFiles.put(file, allFiles.get(file)));
-
- stats = CompactableUtils.compact(tablet, job, cInfo, compactEnv, compactFiles, tmpFileName);
+ // Limit interrupting compactions to the part that reads and writes files and avoid
+ // interrupting the metadata table updates.
+ try (var ignored = new CompactionInterrupter()) {
+ stats = CompactableUtils.compact(tablet, job, cInfo, compactEnv, compactFiles, tmpFileName);
Review Comment:
The main difference is that compactor.interrupt() was not calling Thread.interrupt(). However it could call that, it was already tracking the thread running the compaction. Modified it do that in 7d39c8d moving where Thread.interrupt() is called down a level (this will probably benefit external compactions also). The test still pass w/ this change, however they were a bit slower because the interrupting the thread could take up 10 seconds after calling close(). So changed the scheduled timer to run every 3 seconds. The previous code would immediately interrupt the thread when close() was called.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] implements interuption of compactions and adds an IT to verify compaction cancels on tablet migration [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #5395:
URL: https://github.com/apache/accumulo/pull/5395#issuecomment-2722445296
When merging this from 2.1 to main, dropped the test because it was based on internal compactions which do not exists in main.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] implements interuption of compactions and adds an IT to verify compaction cancels on tablet migration [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner merged PR #5395:
URL: https://github.com/apache/accumulo/pull/5395
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] implements interuption of compactions and adds an IT to verify compaction cancels on tablet migration [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #5395:
URL: https://github.com/apache/accumulo/pull/5395#issuecomment-2717806423
I'm wondering if we should change:
https://github.com/apache/accumulo/blob/29506999476e99ffc15019469c35705ced0d3278/server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactableImpl.java#L1199-L1201
to:
```
public boolean isCompactionEnabled() {
return !Thread.currentThread.isInterrupted() && inexpensiveCheck.get() && expensiveCheck.get();
}
```
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] implements interuption of compactions and adds an IT to verify compaction cancels on tablet migration [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5395:
URL: https://github.com/apache/accumulo/pull/5395#discussion_r1990208020
##########
test/src/main/java/org/apache/accumulo/test/functional/CompactionIT.java:
##########
@@ -797,6 +804,108 @@ public void testGetActiveCompactions() throws Exception {
}
}
+ @Test
+ public void testMigrationCancelCompaction() throws Exception {
+
+ // This test creates 40 tablets w/ slow iterator, causes 40 compactions to start, and then
+ // starts a new tablet server. Some of the tablets should migrate to the new tserver and cancel
+ // their compaction. Because the test uses a slow iterator, if close blocks on compaction then
+ // the test should timeout. Two tables are used to have different iterator settings inorder to
+ // test the two different way compactions can be canceled. Compactions can be canceled by thread
+ // interrupt or by a check that is done after a compaction iterator returns a key value.
+
+ final String[] tables = this.getUniqueNames(2);
+ try (AccumuloClient client = Accumulo.newClient().from(getClientProps()).build()) {
+ client.instanceOperations().setProperty(
+ Property.TSERV_COMPACTION_SERVICE_DEFAULT_EXECUTORS.getKey(),
+ "[{'name':'any','numThreads':20}]".replaceAll("'", "\""));
+
+ SortedSet<Text> splits = IntStream.range(1, 20).mapToObj(i -> String.format("%06d", i * 1000))
+ .map(Text::new).collect(Collectors.toCollection(TreeSet::new));
+
+ // This iterator is intended to cover the case of a compaction being canceled by thread
+ // interrupt.
+ IteratorSetting setting1 = new IteratorSetting(50, "sleepy", SlowIterator.class);
+ setting1.addOption("sleepTime", "300000");
+ setting1.addOption("seekSleepTime", "3000");
+ SlowIterator.sleepUninterruptibly(setting1, false);
+
+ client.tableOperations().create(tables[0], new NewTableConfiguration().withSplits(splits)
+ .attachIterator(setting1, EnumSet.of(IteratorScope.majc)));
+
+ // This iterator is intended to cover the case of compaction being canceled by the check after
+ // a key value is returned. The iterator is configured to ignore interrupts.
+ IteratorSetting setting2 = new IteratorSetting(50, "sleepy", SlowIterator.class);
+ setting2.addOption("sleepTime", "2000");
+ setting2.addOption("seekSleepTime", "2000");
+ SlowIterator.sleepUninterruptibly(setting2, true);
+
+ client.tableOperations().create(tables[1], new NewTableConfiguration().withSplits(splits)
+ .attachIterator(setting2, EnumSet.of(IteratorScope.majc)));
+
+ // write files to each tablet, should cause compactions to start
+ for (var table : tables) {
+ for (int round = 0; round < 5; round++) {
+ try (var writer = client.createBatchWriter(table)) {
+ for (int i = 0; i < 20_000; i++) {
+ Mutation m = new Mutation(String.format("%06d", i));
+ m.put("f", "q", "v");
+ writer.addMutation(m);
+ }
+ }
+ client.tableOperations().flush(table, null, null, true);
+ }
+ }
+
+ assertEquals(2, client.instanceOperations().getTabletServers().size());
+
+ var ctx = (ClientContext) client;
+ var tableId1 = ctx.getTableId(tables[0]);
+ var tableId2 = ctx.getTableId(tables[1]);
+
+ Wait.waitFor(() -> {
+ var runningCompactions = client.instanceOperations().getActiveCompactions().stream()
+ .map(ac -> ac.getTablet().getTable())
+ .filter(tid -> tid.equals(tableId1) || tid.equals(tableId2)).count();
+ log.debug("Running compactions {}", runningCompactions);
+ return runningCompactions == 40;
Review Comment:
This test usually passes for me, every once in a while it will get stuck here w/ less than the expected 40 compactions. Probably some issue w/ the propagation of compaction config changes.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] implements interuption of compactions and adds an IT to verify compaction cancels on tablet migration [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5395:
URL: https://github.com/apache/accumulo/pull/5395#discussion_r1991431321
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactableImpl.java:
##########
@@ -1340,8 +1391,11 @@ public void compact(CompactionServiceId service, CompactionJob job, BooleanSuppl
SortedMap<StoredTabletFile,DataFileValue> allFiles = tablet.getDatafiles();
HashMap<StoredTabletFile,DataFileValue> compactFiles = new HashMap<>();
cInfo.jobFiles.forEach(file -> compactFiles.put(file, allFiles.get(file)));
-
- stats = CompactableUtils.compact(tablet, job, cInfo, compactEnv, compactFiles, tmpFileName);
+ // Limit interrupting compactions to the part that reads and writes files and avoid
+ // interrupting the metadata table updates.
+ try (var ignored = new CompactionInterrupter()) {
+ stats = CompactableUtils.compact(tablet, job, cInfo, compactEnv, compactFiles, tmpFileName);
Review Comment:
I'm trying to understand how this PR is different than what is happening inside CompactableUtils.compact at:
https://github.com/apache/accumulo/blob/29506999476e99ffc15019469c35705ced0d3278/server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactableUtils.java#L573-L579
When `CompactableImpl.close` is called and sets `closed=true`, then `cenv.isCompactionEnabled()` should return `false` and interrupt the FileCompactor. I'm thinking the difference here is checking the `Thread.interrupted` state and throwing an `InterruptedException` in `CompactionInterrupter.close`. I'm wondering if the code below is functionally equivalent:
```
try {
stats = CompactableUtils.compact(tablet, job, cInfo, compactEnv, compactFiles, tmpFileName);
} finally {
if (Thread.interrupted()) {
throw new InterruptedException();
}
}
```
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] implements interuption of compactions and adds an IT to verify compaction cancels on tablet migration [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5395:
URL: https://github.com/apache/accumulo/pull/5395#discussion_r1990206163
##########
test/src/main/java/org/apache/accumulo/test/functional/SlowIterator.java:
##########
@@ -50,21 +51,37 @@ public static void setSeekSleepTime(IteratorSetting is, long t) {
is.addOption(SEEK_SLEEP_TIME, Long.toString(t));
}
+ public static void sleepUninterruptibly(IteratorSetting is, boolean b) {
+ is.addOption(SLEEP_UNINTERRUPTIBLY, Boolean.toString(b));
+ }
+
+ private void sleep(long time) throws IOException {
Review Comment:
I checked these changes by jstacking a tablet server running the new test and verifying that compaction threads were sleeping both ways.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org