Skip to content

Fixes and verification for RFiles located outside of tables directory #6611

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.

Sign up for GitHub

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

Open
dlmarion wants to merge 4 commits into
base: main
Choose a base branch
from
+811 −426
Open

Fixes and verification for RFiles located outside of tables directory #6611

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
74ac872
Fixes and verification for RFiles located outside of tables directory
dlmarion Oct 23, 2026
e40891e
Remove handling for case where no table ids are supplied
dlmarion Oct 23, 2026
e3afbb9
Add ct-ctor-throw suppression
dlmarion Oct 26, 2026
f35ca5e
Update IT to check that no files contain external path
dlmarion Oct 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
142 changes: 11 additions & 131 deletions core/src/main/java/org/apache/accumulo/core/metadata/ReferencedTabletFile.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,10 @@
*/
package org.apache.accumulo.core.metadata;

import static org.apache.accumulo.core.Constants.HDFS_TABLES_DIR;

import java.net.URI;
import java.util.Comparator;
import java.util.Objects;

import org.apache.accumulo.core.data.Range;
import org.apache.accumulo.core.data.TableId;
import org.apache.commons.lang3.StringUtils;
import org.apache.hadoop.fs.Path;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand All @@ -49,114 +44,7 @@
justification = "Constructor validation is required for proper initialization")
public class ReferencedTabletFile extends AbstractTabletFile<ReferencedTabletFile> {

public static class FileParts {

// parts of an absolute URI, like "hdfs://1.2.3.4/accumulo/tables/2a/t-0003/C0004.rf"
// volume: hdfs://1.2.3.4/accumulo
// tableId: 2a
// tabletDir: t-0003
// fileName: C0004.rf
// normalizedPath: hdfs://1.2.3.4/accumulo/tables/2a/t-0003/C0004.rf
private final String volume;
private final TableId tableId;
private final String tabletDir;
private final String fileName;
private final String normalizedPath;

public FileParts(String volume, TableId tableId, String tabletDir, String fileName,
String normalizedPath) {
this.volume = volume;
this.tableId = tableId;
this.tabletDir = tabletDir;
this.fileName = fileName;
this.normalizedPath = normalizedPath;
}

public String getVolume() {
return volume;
}

public TableId getTableId() {
return tableId;
}

public String getTabletDir() {
return tabletDir;
}

public String getFileName() {
return fileName;
}

public String getNormalizedPath() {
return normalizedPath;
}

}

private static String constructErrorMsg(Path filePath) {
return "Missing or invalid part of tablet file metadata entry: " + filePath;
}

public static FileParts parsePath(Path filePath) {
// File name construct: <volume>/<tablePath>/<tableId>/<tablet>/<file>
// Example: hdfs://namenode:9020/accumulo/tables/1/default_tablet/F00001.rf
// Example compaction tmp file:
// hdfs://namenode:9020/accumulo/tables/1/default_tablet/F00001.rf_tmp_ECID-<uuid>
final URI uri = filePath.toUri();

// validate that this is a fully qualified uri
Preconditions.checkArgument(uri.getScheme() != null, constructErrorMsg(filePath));

final String path = uri.getPath(); // ex: /accumulo/tables/1/default_tablet/F00001.rf
final String[] parts = path.split("/");
final int numParts = parts.length; // should contain tables, 1, default_tablet, F00001.rf

if (numParts < 4) {
throw new IllegalArgumentException(constructErrorMsg(filePath));
}

final String fileName = parts[numParts - 1];
final String tabletDirectory = parts[numParts - 2];
final TableId tableId = TableId.of(parts[numParts - 3]);
final String tablesPath = parts[numParts - 4];

// determine where file path starts, the rest is the volume
final String computedFilePath =
HDFS_TABLES_DIR + "/" + tableId.canonical() + "/" + tabletDirectory + "/" + fileName;
final String uriString = uri.toString();
int idx = uriString.lastIndexOf(computedFilePath);

if (idx == -1) {
throw new IllegalArgumentException(constructErrorMsg(filePath));
}

// The volume is the beginning portion of the uri up to the start
// of the file path.
final String volume = uriString.substring(0, idx);

if (StringUtils.isBlank(fileName) || StringUtils.isBlank(tabletDirectory)
|| StringUtils.isBlank(tablesPath) || StringUtils.isBlank(volume)) {
throw new IllegalArgumentException(constructErrorMsg(filePath));
}
ValidationUtil.validateFileName(fileName);
Preconditions.checkArgument(tablesPath.equals(HDFS_TABLES_DIR_NAME),
"tables directory name is not " + HDFS_TABLES_DIR_NAME + ", is " + tablesPath);

final String normalizedPath = volume + computedFilePath;

if (!normalizedPath.equals(uriString)) {
throw new RuntimeException("Error parsing file path, " + normalizedPath + " != " + uriString);
}

return new FileParts(volume, tableId, tabletDirectory, fileName, normalizedPath);

}

private final FileParts parts;

private static final Logger log = LoggerFactory.getLogger(ReferencedTabletFile.class);
private static final String HDFS_TABLES_DIR_NAME = HDFS_TABLES_DIR.substring(1);

private static final Comparator<ReferencedTabletFile> comparator =
Comparator.comparing(ReferencedTabletFile::getNormalizedPathStr)
Expand All @@ -171,34 +59,27 @@ public ReferencedTabletFile(Path metaPath) {
* qualify an absolute path or create a new file.
*/
public ReferencedTabletFile(Path metaPath, Range range) {
super(Objects.requireNonNull(metaPath), range);
log.trace("Parsing TabletFile from {}", metaPath);
parts = parsePath(metaPath);
}

public String getVolume() {
return parts.getVolume();
}

public TableId getTableId() {
return parts.getTableId();
super(validatePath(metaPath), range);
log.trace("Creating tablet file reference from {}", metaPath);
}

public String getTabletDir() {
return parts.getTabletDir();
private static Path validatePath(Path metaPath) {
Path path = ValidationUtil.validate(Objects.requireNonNull(metaPath));
Preconditions.checkArgument(!path.getName().isEmpty(), "Path must identify a file: %s", path);
return path;
}

@Override
public String getFileName() {
return parts.getFileName();
return path.getName();
}

/**
* Return a string for opening and reading the tablet file. Doesn't have to be exact string in
* metadata.
*/
public String getNormalizedPathStr() {
return parts.getNormalizedPath();
return path.toString();
}

/**
Expand All @@ -216,20 +97,19 @@ public int compareTo(ReferencedTabletFile o) {
@Override
public boolean equals(Object obj) {
if (obj instanceof ReferencedTabletFile that) {
return parts.getNormalizedPath().equals(that.parts.getNormalizedPath())
&& range.equals(that.range);
return path.equals(that.path) && range.equals(that.range);
}
return false;
}

@Override
public int hashCode() {
return Objects.hash(parts.getNormalizedPath(), range);
return Objects.hash(path, range);
}

@Override
public String toString() {
return parts.getNormalizedPath();
return path.toString();
}

public static ReferencedTabletFile of(final Path path) {
Expand Down
61 changes: 51 additions & 10 deletions core/src/main/java/org/apache/accumulo/core/metadata/ScanServerRefTabletFile.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,37 +19,43 @@
package org.apache.accumulo.core.metadata;

import java.net.URI;
import java.util.Collection;
import java.util.HashSet;
import java.util.Objects;
import java.util.Set;
import java.util.TreeSet;
import java.util.UUID;

import org.apache.accumulo.core.data.Key;
import org.apache.accumulo.core.data.Mutation;
import org.apache.accumulo.core.data.TableId;
import org.apache.accumulo.core.data.Value;
import org.apache.hadoop.fs.Path;
import org.apache.hadoop.io.Text;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;

@SuppressFBWarnings(value = "CT_CONSTRUCTOR_THROW",
justification = "Constructor validation is required for proper initialization")
public class ScanServerRefTabletFile extends ReferencedTabletFile {

private final Value NULL_VALUE = new Value(new byte[0]);
private final Text serverAddress;
private final Text uuid;
private final Set<TableId> tableIds;

@keith-turner keith-turner Oct 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private final Set<TableId> tableIds;

Traced how table ids are used in the code and found we may be able to remove tracking of table ids for scan server refs if we make the following change. That would simplify this class and avoid persisting data this is not really needed.

diff --git a/server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectionAlgorithm.java b/server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectionAlgorithm.java
index 6b9a4ed23c..b9301d1e25 100644
--- a/server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectionAlgorithm.java
+++ b/server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectionAlgorithm.java
@@ -202,7 +202,11 @@ public class GarbageCollectionAlgorithm {
     Set<TableId> tableIdsSeen = new HashSet<>(tableIdsBefore.size());
     try (Stream<Reference> references = gce.getReferences()) {
       references.forEach(ref -> {
-        tableIdsSeen.add(ref.getTableId());
+        if (ref.isDirectory()) {
+          // Every tablet must have a directory, but not every tablet has files. Only need to look
+          // for directories to ensure a tables tablets were seen.
+          tableIdsSeen.add(ref.getTableId());
+        }
 
         if (ref.isDirectory()) {
           ServerColumnFamily.validateDirCol(ref.getMetadataPath());

The above will avoid getting the table id when its not really needed for what the code is trying to accomplish. The other place that gets the table id is here and that case should never happen for scan refs. That case is for handling a super old relative reference in the metadata table from really old versions of Accumulo, scan server refs have always been absolute. Maybe we can change ReferenceFile to throw an UnsupportedOperationException if trying to get the table id for a scan server reference as its not expected to be called for that case.


public ScanServerRefTabletFile(UUID serverLockUUID, String serverAddress, String file) {
public ScanServerRefTabletFile(UUID serverLockUUID, String serverAddress, String file,
Collection<TableId> tableIds) {
super(new Path(URI.create(file)));
this.serverAddress = new Text(serverAddress);
uuid = new Text(serverLockUUID.toString());
this.tableIds = copyAndValidateTableIds(tableIds);
}

public ScanServerRefTabletFile(String file, String serverAddress, UUID serverLockUUID) {
super(new Path(URI.create(file)));
this.serverAddress = new Text(serverAddress);
this.uuid = new Text(serverLockUUID.toString());
}

public ScanServerRefTabletFile(Key k) {
public ScanServerRefTabletFile(Key k, Value value) {
super(new Path(URI.create(k.getColumnQualifier().toString())));
serverAddress = k.getColumnFamily();
uuid = k.getRow();
tableIds = decodeTableIds(Objects.requireNonNull(value).toString());
}

public Mutation putMutation() {
Expand All @@ -73,7 +79,42 @@ public UUID getServerLockUUID() {
}

public Value getValue() {
return NULL_VALUE;
TreeSet<String> sortedIds = new TreeSet<>();
for (TableId tableId : tableIds) {
sortedIds.add(tableId.canonical());
}
return new Value(String.join(",", sortedIds));
}

public Set<TableId> getTableIds() {
return Set.copyOf(tableIds);
}

private static Set<TableId> decodeTableIds(String serialized) {
if (serialized.isEmpty()) {
throw new IllegalArgumentException("Serialized table IDs must not be empty");
}

Set<TableId> tableIds = new HashSet<>();
for (String tableId : serialized.split(",")) {
if (tableId.isEmpty()) {
throw new IllegalArgumentException("Serialized table IDs must not contain empty IDs");
}
tableIds.add(TableId.of(tableId));
}
return copyAndValidateTableIds(tableIds);
}

private static Set<TableId> copyAndValidateTableIds(Collection<TableId> tableIds) {
Objects.requireNonNull(tableIds, "Table IDs must be supplied");
Set<TableId> copy = new HashSet<>(tableIds);
if (copy.isEmpty()) {
throw new IllegalArgumentException("Table IDs must not be empty");
}
for (TableId tableId : copy) {
Objects.requireNonNull(tableId, "Table IDs must not contain null");
}
return copy;
}

public Text getServerAddress() {
Expand Down
5 changes: 0 additions & 5 deletions core/src/main/java/org/apache/accumulo/core/metadata/StoredTabletFile.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@

import org.apache.accumulo.core.data.Key;
import org.apache.accumulo.core.data.Range;
import org.apache.accumulo.core.data.TableId;
import org.apache.accumulo.core.util.json.ByteArrayToBase64TypeAdapter;
import org.apache.hadoop.fs.Path;
import org.apache.hadoop.io.DataInputBuffer;
Expand Down Expand Up @@ -109,10 +108,6 @@ public ReferencedTabletFile getTabletFile() {
return referencedTabletFile;
}

public TableId getTableId() {
return referencedTabletFile.getTableId();
}

@Override
public String getFileName() {
return referencedTabletFile.getFileName();
Expand Down
66 changes: 66 additions & 0 deletions core/src/test/java/org/apache/accumulo/core/metadata/ScanServerRefTabletFileTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/*
* 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.core.metadata;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;

import java.util.Set;
import java.util.UUID;

import org.apache.accumulo.core.data.Key;
import org.apache.accumulo.core.data.TableId;
import org.apache.accumulo.core.data.Value;
import org.apache.hadoop.io.Text;
import org.junit.jupiter.api.Test;

public class ScanServerRefTabletFileTest {

@Test
public void tableIdsRoundTripForExternalPaths() {
UUID serverLockUuid = UUID.randomUUID();
String serverAddress = "localhost:1234";
String file = "hdfs://nn/external/rfiles/source.rf";
Set<TableId> tableIds = Set.of(TableId.of("2a"), TableId.of("3b"));
var reference = new ScanServerRefTabletFile(serverLockUuid, serverAddress, file, tableIds);
var key = new Key(new Text(serverLockUuid.toString()), new Text(serverAddress),
reference.getFilePath(), new Text(), 1);

var decoded = new ScanServerRefTabletFile(key, reference.getValue());

assertEquals(file, decoded.getNormalizedPathStr());
assertEquals(tableIds, decoded.getTableIds());
}

@Test
public void tableIdsAreRequired() {
String serverAddress = "localhost:1234";
String file = "hdfs://nn/external/rfiles/source.rf";

assertThrows(NullPointerException.class,
() -> new ScanServerRefTabletFile(UUID.randomUUID(), serverAddress, file, null));
assertThrows(IllegalArgumentException.class,
() -> new ScanServerRefTabletFile(UUID.randomUUID(), serverAddress, file, Set.of()));

var key = new Key(new Text(UUID.randomUUID().toString()), new Text(serverAddress),
new Text(file), new Text(), 1);
assertThrows(IllegalArgumentException.class,
() -> new ScanServerRefTabletFile(key, new Value()));
}
}
Loading
Loading