-
Notifications
You must be signed in to change notification settings - Fork 480
Removes in memory set from dead compaction detector #6283
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
2b6cbb6
6ad2090
8e895f7
b211e5b
d5af542
3ce74c0
4993169
db57334
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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())); | ||
|
keith-turner marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| private void write(Collection<Ample.RemovedCompaction> removedCompactions, | ||
| Function<Ample.RemovedCompaction,Mutation> converter) { | ||
| if (removedCompactions.isEmpty()) { | ||
| return; | ||
| } | ||
|
|
||
| Map<Ample.DataLevel,List<Ample.RemovedCompaction>> byLevel = removedCompactions.stream() | ||
| .collect(Collectors.groupingBy(rc -> Ample.DataLevel.of(rc.table()))); | ||
| // Do not expect the root to split or merge so it should never have this data | ||
| Preconditions.checkArgument(!byLevel.containsKey(Ample.DataLevel.ROOT)); | ||
| byLevel.forEach((dl, removed) -> { | ||
| try (var writer = context.createBatchWriter(dl.metaTable())) { | ||
| for (var rc : removed) { | ||
| writer.addMutation(converter.apply(rc)); | ||
| } | ||
| } catch (TableNotFoundException | MutationsRejectedException e) { | ||
| throw new IllegalStateException(e); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| @Override | ||
| public void add(Collection<Ample.RemovedCompaction> removedCompactions) { | ||
| write(removedCompactions, rc -> { | ||
| Mutation m = new Mutation(RemovedCompactionSection.encodeRow(rc)); | ||
| m.put("", "", ""); | ||
| return m; | ||
| }); | ||
|
|
||
| } | ||
|
|
||
| @Override | ||
| public void delete(Collection<Ample.RemovedCompaction> removedCompactions) { | ||
| write(removedCompactions, rc -> { | ||
| Mutation m = new Mutation(RemovedCompactionSection.encodeRow(rc)); | ||
| m.putDelete("", ""); | ||
| return m; | ||
| }); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,11 +18,13 @@ | |
| */ | ||
| package org.apache.accumulo.server.util; | ||
|
|
||
| import java.io.FileNotFoundException; | ||
| import java.io.IOException; | ||
| import java.util.ArrayList; | ||
| import java.util.Arrays; | ||
| import java.util.Collection; | ||
| import java.util.Iterator; | ||
| import java.util.LinkedList; | ||
| import java.util.List; | ||
| import java.util.Set; | ||
| import java.util.concurrent.ConcurrentSkipListSet; | ||
|
|
@@ -32,7 +34,9 @@ | |
| import java.util.concurrent.Future; | ||
| import java.util.concurrent.TimeUnit; | ||
|
|
||
| import org.apache.accumulo.core.Constants; | ||
| import org.apache.accumulo.core.cli.ServerOpts; | ||
| import org.apache.accumulo.core.data.TableId; | ||
| import org.apache.accumulo.core.metadata.schema.Ample.DataLevel; | ||
| import org.apache.accumulo.core.metadata.schema.ExternalCompactionId; | ||
| import org.apache.accumulo.core.metadata.schema.TabletMetadata.ColumnType; | ||
|
|
@@ -46,6 +50,7 @@ | |
| import org.apache.accumulo.start.spi.CommandGroups; | ||
| import org.apache.accumulo.start.spi.KeywordExecutable; | ||
| import org.apache.hadoop.fs.FileStatus; | ||
| import org.apache.hadoop.fs.FileSystem; | ||
| import org.apache.hadoop.fs.Path; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
@@ -127,6 +132,7 @@ public static Set<Path> findTempFiles(ServerContext context, String tableId) | |
| }); | ||
| } | ||
| } | ||
|
|
||
| LOG.trace("Final set of compaction tmp files after removing active compactions: {}", matches); | ||
| return matches; | ||
| } | ||
|
|
@@ -141,7 +147,8 @@ public static DeleteStats deleteTempFiles(ServerContext context, Set<Path> files | |
| throws InterruptedException { | ||
|
|
||
| final ExecutorService delSvc = Executors.newFixedThreadPool(8); | ||
| final List<Future<Boolean>> futures = new ArrayList<>(filesToDelete.size()); | ||
| // use a linked list to make removal from the middle of the list quick | ||
| final List<Future<Boolean>> futures = new LinkedList<>(); | ||
| final DeleteStats stats = new DeleteStats(); | ||
|
|
||
| filesToDelete.forEach(p -> { | ||
|
|
@@ -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, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some of this code seems like it has overlap with
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Made a change in 4993169 to consolidate the delete code. Still needed a separate function to find tmp files. |
||
| Set<ExternalCompactionId> ecidsForTablet) { | ||
| final Collection<Volume> vols = ctx.getVolumeManager().getVolumes(); | ||
| for (Volume vol : vols) { | ||
| try { | ||
| final String volPath = vol.getBasePath() + Constants.HDFS_TABLES_DIR + Path.SEPARATOR | ||
| + tableId.canonical() + Path.SEPARATOR + dirName; | ||
| final FileSystem fs = vol.getFileSystem(); | ||
| for (ExternalCompactionId ecid : ecidsForTablet) { | ||
| final String fileSuffix = "_tmp_" + ecid.canonical(); | ||
| FileStatus[] files = null; | ||
| try { | ||
| files = fs.listStatus(new Path(volPath), (path) -> path.getName().endsWith(fileSuffix)); | ||
| } catch (FileNotFoundException e) { | ||
| LOG.trace("Failed to list tablet dir {}", volPath, e); | ||
| } | ||
| if (files != null) { | ||
| for (FileStatus file : files) { | ||
| if (!fs.delete(file.getPath(), false)) { | ||
| LOG.warn("Unable to delete ecid tmp file: {}: ", file.getPath()); | ||
| } else { | ||
| LOG.debug("Deleted ecid tmp file: {}", file.getPath()); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } catch (IOException e) { | ||
| LOG.error("Exception deleting compaction tmp files for table: {}", tableId, e); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public FindCompactionTmpFiles() { | ||
| super(new FindOpts()); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took me a few minutes to understand what a
RemovedCompactionrepresented. 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
OrphanedCompactionbe a better name?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OrphanedCompactionis a much better name, will change to that. Used removed in the name because it was removed from the metadata table.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that is all correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed in db57334