dkranchii opened a new pull request, #19242: URL: https://github.com/apache/pinot/pull/19242
## Summary `LocalLogFileServer.getAllLogFilePaths()` enumerated log files via `Files.walk(_logRootDirPath).filter(...).forEach(...)` without a `try-with-resources` block. Per the [JDK `Files.walk` javadoc](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/nio/file/Files.html#walk(java.nio.file.Path,java.nio.file.FileVisitOption...)): > The returned stream encapsulates one or more `DirectoryStream`s. If timely disposal of file system resources is required, the try-with-resources construct should be used… Because `downloadLogFile(String)` calls `getAllLogFilePaths()` on **every** download request (to authorize the requested path), the leak amplifies on hot paths — long-lived server and controller instances gradually accumulate `DirectoryStream` file descriptors until they approach the process `ulimit -n`. This PR wraps the `Files.walk` stream in a `try-with-resources` block so the underlying `DirectoryStream`(s) are released as soon as enumeration completes. Sibling code in Pinot already uses the same pattern (e.g. `LocalPinotFS.listFiles`). ## Change - `pinot-common/src/main/java/org/apache/pinot/common/utils/log/LocalLogFileServer.java` — wrap `Files.walk(_logRootDirPath)` in `try (Stream<Path> paths = Files.walk(...))`; enumeration logic is unchanged. ## Backwards compatibility None affected. Public API, return values, and enumeration behavior are unchanged. ## Tests - Added `LocalLogFileServerTest#testGetAllLogFilePathsEnumeratesNestedDirectories` — creates a nested `sub/dir/nested.log`, asserts both files are enumerated with paths relative to the log root, and asserts both are downloadable via `downloadLogFile(...)`. This guards the refactor against a regression that would break recursion into subdirectories. - The pre-existing `testLoggerFileServer` continues to exercise the flat-directory happy path and the `FORBIDDEN` response for unknown paths. Run locally: ```bash ./mvnw -pl pinot-common -am -Dtest=LocalLogFileServerTest test ``` ## Risk Very low. The change is a mechanical `try-with-resources` wrap around an existing `Files.walk` invocation; the traversal semantics are unchanged. The stream is fully consumed inside the block, so no lazy operations escape. -- 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]
