This is an automated email from the ASF dual-hosted git repository.
Jackie-Jiang pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git
The following commit(s) were added to refs/heads/master by this push:
new fdebb59d7b4 Throw IOException instead of NPE when listing an invalid
path in LocalPinotFS (#19250)
fdebb59d7b4 is described below
commit fdebb59d7b4ebae4f5e6690c1f9935036a6d5649
Author: Xiaotian (Jackie) Jiang <[email protected]>
AuthorDate: Thu Aug 13 12:42:29 2026 -0700
Throw IOException instead of NPE when listing an invalid path in
LocalPinotFS (#19250)
---
.../helix/core/retention/RetentionManager.java | 11 ++++++++++-
.../helix/core/retention/RetentionManagerTest.java | 6 ++++++
.../apache/pinot/spi/filesystem/LocalPinotFS.java | 20 ++++++++++++++++++--
.../pinot/spi/filesystem/LocalPinotFSTest.java | 21 +++++++++++++++++++++
4 files changed, 55 insertions(+), 3 deletions(-)
diff --git
a/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/retention/RetentionManager.java
b/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/retention/RetentionManager.java
index 79bad7f046d..b5ba82d3133 100644
---
a/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/retention/RetentionManager.java
+++
b/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/retention/RetentionManager.java
@@ -378,7 +378,7 @@ public class RetentionManager extends
ControllerPeriodicTask<Void> {
}
} catch (IOException e) {
LOGGER.warn("Unable to fetch segments from deep store that are beyond
retention period for table: {}",
- tableNameWithType);
+ tableNameWithType, e);
}
return segmentsToDelete;
@@ -406,6 +406,15 @@ public class RetentionManager extends
ControllerPeriodicTask<Void> {
URI tableDataUri =
URIUtils.getUri(_pinotHelixResourceManager.getDataDir(), rawTableName);
PinotFS pinotFS = PinotFSFactory.create(tableDataUri.getScheme());
+ // The data dir is created when the first segment is pushed, so it is
legitimately absent for a table that has
+ // never had a segment in deep store. Such a table has no untracked
segments to delete, and listing a
+ // non-existent directory fails on most file systems.
+ if (!pinotFS.exists(tableDataUri)) {
+ LOGGER.info("Skipping deep store scan for untracked segments for table:
{} as data dir: {} does not exist",
+ tableNameWithType, tableDataUri);
+ return segmentsToDelete;
+ }
+
long startTimeMs = System.currentTimeMillis();
List<FileMetadata> deepstoreFiles =
pinotFS.listFilesWithMetadata(tableDataUri, false);
diff --git
a/pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/retention/RetentionManagerTest.java
b/pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/retention/RetentionManagerTest.java
index 349e7d18e0b..7c1cf544a04 100644
---
a/pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/retention/RetentionManagerTest.java
+++
b/pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/retention/RetentionManagerTest.java
@@ -966,6 +966,12 @@ public class RetentionManagerTest {
public static class FakePinotFs extends LocalPinotFS {
+ @Override
+ public boolean exists(URI fileUri) {
+ // The fake deep store is not backed by a real directory, but always has
the segments listed below
+ return true;
+ }
+
@Override
public List<FileMetadata> listFilesWithMetadata(URI fileUri, boolean
recursive)
throws IOException {
diff --git
a/pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/LocalPinotFS.java
b/pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/LocalPinotFS.java
index 2d312a07fce..dd01a16f616 100644
--- a/pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/LocalPinotFS.java
+++ b/pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/LocalPinotFS.java
@@ -115,7 +115,8 @@ public class LocalPinotFS extends BasePinotFS {
throws IOException {
File file = toFile(fileUri);
if (!recursive) {
- return Arrays.stream(file.list()).map(s -> new File(file,
s)).map(File::getAbsolutePath).toArray(String[]::new);
+ return Arrays.stream(listFileNames(file)).map(s -> new File(file,
s)).map(File::getAbsolutePath)
+ .toArray(String[]::new);
} else {
try (Stream<Path> pathStream = Files.walk(Paths.get(fileUri))) {
return pathStream.filter(s ->
!s.equals(file.toPath())).map(Path::toString).toArray(String[]::new);
@@ -128,7 +129,8 @@ public class LocalPinotFS extends BasePinotFS {
throws IOException {
File file = toFile(fileUri);
if (!recursive) {
- return Arrays.stream(file.list()).map(s -> getFileMetadata(new
File(file, s))).collect(Collectors.toList());
+ return Arrays.stream(listFileNames(file)).map(s -> getFileMetadata(new
File(file, s)))
+ .collect(Collectors.toList());
} else {
try (Stream<Path> pathStream = Files.walk(Paths.get(fileUri))) {
return pathStream.filter(s -> !s.equals(file.toPath()))
@@ -138,6 +140,20 @@ public class LocalPinotFS extends BasePinotFS {
}
}
+ /// Returns the names of the entries directly under the given directory.
+ ///
+ /// [File#list()] returns `null` instead of throwing when the path does not
exist, is not a directory, or cannot be
+ /// read, so translate that into the `IOException` the listing methods are
contracted to throw for an invalid path.
+ private static String[] listFileNames(File file)
+ throws IOException {
+ String[] fileNames = file.list();
+ if (fileNames == null) {
+ throw new IOException("Failed to list files under: " +
file.getAbsolutePath()
+ + " because it does not exist, is not a directory, or cannot be
read");
+ }
+ return fileNames;
+ }
+
private static FileMetadata getFileMetadata(File file) {
return new FileMetadata.Builder().setFilePath(file.getAbsolutePath())
.setLastModifiedTime(file.lastModified())
diff --git
a/pinot-spi/src/test/java/org/apache/pinot/spi/filesystem/LocalPinotFSTest.java
b/pinot-spi/src/test/java/org/apache/pinot/spi/filesystem/LocalPinotFSTest.java
index 7b3338bd111..4ada83680e5 100644
---
a/pinot-spi/src/test/java/org/apache/pinot/spi/filesystem/LocalPinotFSTest.java
+++
b/pinot-spi/src/test/java/org/apache/pinot/spi/filesystem/LocalPinotFSTest.java
@@ -310,4 +310,25 @@ public class LocalPinotFSTest {
expectedRecursive.containsAll(fileMetadata.stream().map(FileMetadata::getFilePath).collect(Collectors.toSet())),
fileMetadata.toString());
}
+
+ @Test
+ public void testListFilesOnPathThatIsNotADirectory()
+ throws IOException {
+ LocalPinotFS localPinotFS = new LocalPinotFS();
+ File tempDirPath = new File(_absoluteTmpDirPath,
"test-list-files-invalid-path");
+ Assert.assertTrue(tempDirPath.mkdirs());
+
+ File nonExistentDir = new File(tempDirPath, "nonExistentDir");
+ URI nonExistentUri = nonExistentDir.toURI();
+ Assert.assertThrows(IOException.class, () ->
localPinotFS.listFiles(nonExistentUri, false));
+ Assert.assertThrows(IOException.class, () ->
localPinotFS.listFilesWithMetadata(nonExistentUri, false));
+ Assert.assertThrows(IOException.class, () ->
localPinotFS.listFiles(nonExistentUri, true));
+ Assert.assertThrows(IOException.class, () ->
localPinotFS.listFilesWithMetadata(nonExistentUri, true));
+
+ File testFile = new File(tempDirPath, "testFile");
+ Assert.assertTrue(testFile.createNewFile());
+ URI fileUri = testFile.toURI();
+ Assert.assertThrows(IOException.class, () ->
localPinotFS.listFiles(fileUri, false));
+ Assert.assertThrows(IOException.class, () ->
localPinotFS.listFilesWithMetadata(fileUri, false));
+ }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
