Jackie-Jiang opened a new pull request, #19250:
URL: https://github.com/apache/pinot/pull/19250
## Summary
`File.list()` returns `null` — instead of throwing — when the path does not
exist, is not a directory, or cannot be read. The non-recursive branches of
`LocalPinotFS.listFiles()` and `LocalPinotFS.listFilesWithMetadata()` passed
that result straight into `Arrays.stream(...)`, so an invalid path surfaced as
a `NullPointerException` rather than the `IOException` the `PinotFS` listing
contract specifies:
> Throws IOException if this abstract pathname is not valid, or if an I/O
error occurs.
The recursive branches already behaved correctly — `Files.walk` throws
`NoSuchFileException`.
This shows up in `RetentionManager`, which scans deep store for untracked
segments. That call site handles `IOException`, but the NPE escaped the handler
and was caught by the periodic-task loop instead:
```
ERROR [ControllerPeriodicTask] Caught exception while processing table:
myTable_OFFLINE in task: RetentionManager
java.lang.NullPointerException: Cannot read the array length because "array"
is null
at java.base/java.util.Arrays.stream(Arrays.java:5464)
at
org.apache.pinot.spi.filesystem.LocalPinotFS.listFilesWithMetadata(LocalPinotFS.java:131)
at
org.apache.pinot.controller.helix.core.retention.RetentionManager.findUntrackedSegmentsToDeleteFromDeepstore(RetentionManager.java:411)
...
```
It reproduces for any table whose data dir does not exist yet, since
`<dataDir>/<rawTableName>` is only created once the first segment lands in deep
store.
## Changes
- `LocalPinotFS`: route both non-recursive listing branches through a
`listFileNames` helper that turns the `null` return into an `IOException`
naming the path.
- `RetentionManager`: skip the deep-store scan when the table data dir does
not exist. Such a table has no untracked segments, and without this the fixed
`IOException` would produce a warning on every retention cycle for every table
that has never had a segment pushed.
- `RetentionManager`: the existing `catch (IOException)` logged a message
without the exception — pass it along so a genuine listing failure is
diagnosable.
- `LocalPinotFSTest`: new `testListFilesOnPathThatIsNotADirectory`, covering
a missing directory and a regular file for both listing methods. It fails with
an NPE without the fix.
- `RetentionManagerTest`: `FakePinotFs` now overrides `exists()`, since it
serves a `fake://` URI with no real directory behind it. Without the override
the new early return would silently turn
`testPerformanceWithLargeNumberOfSegments` into a no-op.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
