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 2026/03/31 23:00:40 UTC
[PR] Removes in memory set from dead compaction detector [accumulo]
keith-turner opened a new pull request, #6283:
URL: https://github.com/apache/accumulo/pull/6283
Removed an in memory set of tables ids in the dead compaction detectors that contained table ids that may have compaction tmp files that needed cleanup. This set would be hard to maintain in multiple managers. Also the set could lose track of tables if the process died.
Replaced the in memory set with a set in the metadata table. This set is directly populated by the split and merge fate operations, so there is no chance of losing track of things when a process dies. Also this set is more narrow and allows looking for tmp files to cleanup in single tablets dirs rather than scanning an entire tables dir.
Also made a change to the order in which tmp files are deleted for failed compactions. They used to be deleted after the metadata for the compaction was cleaned up, this could lead to losing track of the cleanup if the process died after deleting the metadata but before deleting the tmp file. Now the tmp files are deleted before the metadata entry, so should no longer lose track in process death.
This change is needed by #6217
--
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] Removes in memory set from dead compaction detector [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #6283:
URL: https://github.com/apache/accumulo/pull/6283#discussion_r3046165777
##########
server/base/src/main/java/org/apache/accumulo/server/metadata/RemovedCompactionStoreImpl.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.server.metadata;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import org.apache.accumulo.core.client.MutationsRejectedException;
+import org.apache.accumulo.core.client.Scanner;
+import org.apache.accumulo.core.client.TableNotFoundException;
+import org.apache.accumulo.core.data.Mutation;
+import org.apache.accumulo.core.metadata.SystemTables;
+import org.apache.accumulo.core.metadata.schema.Ample;
+import org.apache.accumulo.core.metadata.schema.MetadataSchema.RemovedCompactionSection;
+import org.apache.accumulo.core.security.Authorizations;
+import org.apache.accumulo.server.ServerContext;
+
+import com.google.common.base.Preconditions;
+
+public class RemovedCompactionStoreImpl implements Ample.RemovedCompactionStore {
+ private final ServerContext context;
+
+ public RemovedCompactionStoreImpl(ServerContext context) {
+ this.context = context;
+ }
+
+ private Stream<Ample.RemovedCompaction> createStream(String tableName) {
+ Scanner scanner = null;
+ try {
+ scanner = context.createScanner(tableName, Authorizations.EMPTY);
+ } catch (TableNotFoundException e) {
+ throw new IllegalStateException(e);
+ }
+ scanner.setRange(RemovedCompactionSection.getRange());
+ return scanner.stream().map(e -> e.getKey().getRowData().toString())
+ .map(RemovedCompactionSection::decodeRow).onClose(scanner::close);
+ }
+
+ @Override
+ public Stream<Ample.RemovedCompaction> list() {
+ return Stream.concat(createStream(SystemTables.ROOT.tableName()),
+ createStream(SystemTables.METADATA.tableName()));
Review Comment:
This code already uses DataLevels.metaTable() calls in the write method so make it consistent to avoid confusion.
```suggestion
public Stream<Ample.RemovedCompaction> list() {
return Stream.concat(createStream(Ample.DataLevel.METADATA.metaTable()),
createStream(Ample.DataLevel.USER.metaTable()));
```
##########
server/manager/src/main/java/org/apache/accumulo/manager/compaction/coordinator/DeadCompactionDetector.java:
##########
@@ -162,6 +154,31 @@ private void detectDeadCompactions() {
});
}
+ // Get the list of compaction entires that were removed from the metadata table by a split or
Review Comment:
```suggestion
// Get the list of compaction entries that were removed from the metadata table by a split or
```
##########
server/base/src/main/java/org/apache/accumulo/server/util/FindCompactionTmpFiles.java:
##########
@@ -190,6 +197,39 @@ public static DeleteStats deleteTempFiles(ServerContext context, Set<Path> files
return stats;
}
+ // Finds any tmp files matching the given compaction ids in table dir and deletes them.
+ public static void deleteTmpFiles(ServerContext ctx, TableId tableId, String dirName,
Review Comment:
Some of this code seems like it has overlap with `findTempFiles`
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/merge/MergeTablets.java:
##########
@@ -141,6 +142,19 @@ public Repo<FateEnv> call(FateId fateId, FateEnv env) throws Exception {
dirs.clear();
}
}
+
+ // These compaction metadata entries will be deleted, queue up removal of the tmp file once
+ // the compaction is no longer running
+ tabletMeta.getExternalCompactions().keySet().stream()
+ .map(ecid -> new Ample.RemovedCompaction(ecid, tabletMeta.getExtent().tableId(),
+ tabletMeta.getDirName()))
+ .forEach(removedCompactions::add);
+ if (removedCompactions.size() > 1000 && tabletsSeen > 1) {
Review Comment:
What's the significance of the `> 1000` check here and in the gcCandidate creations for Dirs?
Is this just related to a max batch size ?
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/merge/MergeTablets.java:
##########
Review Comment:
There's another deleteExternalCompaction call on lastTabletMeta (line 201).
Do those deleted compactions also need to be added to the removed list?
--
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] Removes in memory set from dead compaction detector [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #6283:
URL: https://github.com/apache/accumulo/pull/6283#discussion_r3047311486
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/merge/MergeTablets.java:
##########
Review Comment:
No those are ok, they were added earlier. The first loop over tablet metdata collects data about tablets, including the last tablet in the range, and it queues up adding the removal markers. At line 201 it changing the last tablet in the merge range to expand it and it also deletes any external 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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removes in memory set from dead compaction detector [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #6283:
URL: https://github.com/apache/accumulo/pull/6283#discussion_r3051090896
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/Ample.java:
##########
@@ -669,4 +670,24 @@ default void removeBulkLoadInProgressFlag(String path) {
default ScanServerRefStore scanServerRefs() {
throw new UnsupportedOperationException();
}
+
+ record RemovedCompaction(ExternalCompactionId id, TableId table, String dir) {
+ };
+
+ /**
+ * Tracks compactions that were removed from the metadata table but may still be running on
+ * compactors. The tmp files associated with these compactions can eventually be removed when the
+ * compaction is no longer running.
+ */
+ interface RemovedCompactionStore {
+ Stream<RemovedCompaction> list();
+
+ void add(Collection<RemovedCompaction> removedCompactions);
Review Comment:
It took me a few minutes to understand what a `RemovedCompaction` represented. My understanding of these changes are that when a tablet is merged or split, the ECID entries in the tablet metadata for the tablets involved are removed. However, the compaction is likely running on a Compactor. Is that right?
I was having trouble understanding the context given the name. I wonder if a different name might better reflect the situation. The compaction itself is not removed, it's OBE. It's been orphaned from it's parent tablet or it's like a dangling ref in a database. Would `OrphanedCompaction` be a better name?
--
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] Removes in memory set from dead compaction detector [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #6283:
URL: https://github.com/apache/accumulo/pull/6283#discussion_r3052519584
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/Ample.java:
##########
@@ -669,4 +670,24 @@ default void removeBulkLoadInProgressFlag(String path) {
default ScanServerRefStore scanServerRefs() {
throw new UnsupportedOperationException();
}
+
+ record RemovedCompaction(ExternalCompactionId id, TableId table, String dir) {
+ };
+
+ /**
+ * Tracks compactions that were removed from the metadata table but may still be running on
+ * compactors. The tmp files associated with these compactions can eventually be removed when the
+ * compaction is no longer running.
+ */
+ interface RemovedCompactionStore {
+ Stream<RemovedCompaction> list();
+
+ void add(Collection<RemovedCompaction> removedCompactions);
Review Comment:
Renamed in db57334
--
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] Removes in memory set from dead compaction detector [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #6283:
URL: https://github.com/apache/accumulo/pull/6283#discussion_r3018932906
##########
server/manager/src/main/java/org/apache/accumulo/manager/compaction/coordinator/CompactionCoordinator.java:
##########
@@ -791,78 +803,18 @@ public boolean test(TabletMetadata tabletMetadata) {
}
});
- final List<ExternalCompactionId> ecidsForTablet = new ArrayList<>();
tabletsMutator.process().forEach((extent, result) -> {
if (result.getStatus() != Ample.ConditionalResult.Status.ACCEPTED) {
-
// this should try again later when the dead compaction detector runs, lets log it in case
// its a persistent problem
if (LOG.isDebugEnabled()) {
LOG.debug("Unable to remove failed compaction {} {}", extent, compactions.get(extent));
}
- } else {
- // compactionFailed is called from the Compactor when either a compaction fails or
- // is cancelled and it's called from the DeadCompactionDetector. This block is
- // entered when the conditional mutator above successfully deletes an ecid from
- // the tablet metadata. Remove compaction tmp files from the tablet directory
- // that have a corresponding ecid in the name.
-
- ecidsForTablet.clear();
- ecidsForTablet.addAll(compactions.get(extent));
-
- if (!ecidsForTablet.isEmpty()) {
- final TabletMetadata tm = ctx.getAmple().readTablet(extent, ColumnType.DIR);
- if (tm != null) {
- final Collection<Volume> vols = ctx.getVolumeManager().getVolumes();
- for (Volume vol : vols) {
Review Comment:
This code was moved to FindCompactionTmpFiles so it could be called here and in the dead compaction detector.
--
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] Removes in memory set from dead compaction detector [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #6283:
URL: https://github.com/apache/accumulo/pull/6283#discussion_r3046858307
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/merge/MergeTablets.java:
##########
@@ -141,6 +142,19 @@ public Repo<FateEnv> call(FateId fateId, FateEnv env) throws Exception {
dirs.clear();
}
}
+
+ // These compaction metadata entries will be deleted, queue up removal of the tmp file once
+ // the compaction is no longer running
+ tabletMeta.getExternalCompactions().keySet().stream()
+ .map(ecid -> new Ample.RemovedCompaction(ecid, tabletMeta.getExtent().tableId(),
+ tabletMeta.getDirName()))
+ .forEach(removedCompactions::add);
+ if (removedCompactions.size() > 1000 && tabletsSeen > 1) {
Review Comment:
That check is to avoid buffering too much data in memory.
--
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] Removes in memory set from dead compaction detector [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #6283:
URL: https://github.com/apache/accumulo/pull/6283#discussion_r3046866212
##########
server/base/src/main/java/org/apache/accumulo/server/util/FindCompactionTmpFiles.java:
##########
@@ -190,6 +197,39 @@ public static DeleteStats deleteTempFiles(ServerContext context, Set<Path> files
return stats;
}
+ // Finds any tmp files matching the given compaction ids in table dir and deletes them.
+ public static void deleteTmpFiles(ServerContext ctx, TableId tableId, String dirName,
Review Comment:
Some code I copied mostly as is from CompactionCoordinator to this class for deleting a file. Did not look at the existing code when I copied it in, will take a look at that.
--
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] Removes in memory set from dead compaction detector [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #6283:
URL: https://github.com/apache/accumulo/pull/6283#discussion_r3047316272
##########
server/base/src/main/java/org/apache/accumulo/server/util/FindCompactionTmpFiles.java:
##########
@@ -190,6 +197,39 @@ public static DeleteStats deleteTempFiles(ServerContext context, Set<Path> files
return stats;
}
+ // Finds any tmp files matching the given compaction ids in table dir and deletes them.
+ public static void deleteTmpFiles(ServerContext ctx, TableId tableId, String dirName,
Review Comment:
Made a change in 4993169 to consolidate the delete code. Still needed a separate function to find tmp files.
--
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] Removes in memory set from dead compaction detector [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #6283:
URL: https://github.com/apache/accumulo/pull/6283#discussion_r3052157388
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/Ample.java:
##########
@@ -669,4 +670,24 @@ default void removeBulkLoadInProgressFlag(String path) {
default ScanServerRefStore scanServerRefs() {
throw new UnsupportedOperationException();
}
+
+ record RemovedCompaction(ExternalCompactionId id, TableId table, String dir) {
+ };
+
+ /**
+ * Tracks compactions that were removed from the metadata table but may still be running on
+ * compactors. The tmp files associated with these compactions can eventually be removed when the
+ * compaction is no longer running.
+ */
+ interface RemovedCompactionStore {
+ Stream<RemovedCompaction> list();
+
+ void add(Collection<RemovedCompaction> removedCompactions);
Review Comment:
`OrphanedCompaction` is a much better name, will change to that. Used removed in the name because it was removed from the metadata table.
--
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] Removes in memory set from dead compaction detector [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #6283:
URL: https://github.com/apache/accumulo/pull/6283#discussion_r3052383307
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/Ample.java:
##########
@@ -669,4 +670,24 @@ default void removeBulkLoadInProgressFlag(String path) {
default ScanServerRefStore scanServerRefs() {
throw new UnsupportedOperationException();
}
+
+ record RemovedCompaction(ExternalCompactionId id, TableId table, String dir) {
+ };
+
+ /**
+ * Tracks compactions that were removed from the metadata table but may still be running on
+ * compactors. The tmp files associated with these compactions can eventually be removed when the
+ * compaction is no longer running.
+ */
+ interface RemovedCompactionStore {
+ Stream<RemovedCompaction> list();
+
+ void add(Collection<RemovedCompaction> removedCompactions);
Review Comment:
> Is that right?
Yes that is all correct.
--
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] Removes in memory set from dead compaction detector [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner merged PR #6283:
URL: https://github.com/apache/accumulo/pull/6283
--
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