mayankshriv opened a new pull request, #19378: URL: https://github.com/apache/pinot/pull/19378
## Problem `ProtoBufUtils.getFileCopiedToLocal()` created temporary directories with prefix `pinot-protobuf` that were never cleaned up. Every call to `ProtoBufMessageDecoder.init()`, `ProtoBufRecordReader.init()`, or `ProtoBufCodeGenMessageDecoder.init()` leaked one directory under `java.io.tmpdir`. ## Fix **Descriptor file callers** (`ProtoBufMessageDecoder`, `ProtoBufRecordReader`): replaced the copy-to-temp-then-open pattern with `readDescriptorFileBytes()` which streams bytes directly via `PinotFS.open()` - no temp files at all. **JAR caller** (`ProtoBufCodeGenMessageDecoder`): resolve the JAR to a local `File` inline. Local JARs are used directly without copying. Remote JARs are copied to a temp directory that intentionally persists for the decoder's lifetime because the JVM may lazily resolve classes from the JAR at decode time via the `URLClassLoader` chain. Since `StreamMessageDecoder` does not extend `Closeable`, there is no lifecycle hook to clean up, but this is a one-time-per-consumer-init cost and consumers are long-lived. Removed dead methods: `getFileCopiedToLocal()`, `getDescriptorFileInputStream()`, `createLocalFile()`, `withLocalFile()`, and `FileAction` from `ProtoBufUtils`. Renamed `loadClass(File)` to `createClassLoader(File)` for clarity. ## Testing Added `ProtoBufTempFileLeakTest` with 5 tests covering all three code paths. Each test snapshots `pinot-protobuf*` directories in `java.io.tmpdir` before the operation, performs the operation with functional correctness assertions (decoding a message and checking field values), then asserts no new temp directories remain afterward. | Test | Path covered | |---|---| | `testMessageDecoderInitDoesNotLeakTempDir` | Simple descriptor via `ProtoBufMessageDecoder` | | `testMessageDecoderComplexDescriptorDoesNotLeakTempDir` | Complex nested descriptor via `ProtoBufMessageDecoder` | | `testCodeGenDecoderInitDoesNotLeakTempDir` | Simple JAR via `ProtoBufCodeGenMessageDecoder` | | `testCodeGenDecoderComplexJarDoesNotLeakTempDir` | Complex JAR with nested/repeated/map types | | `testRecordReaderLifecycleDoesNotLeakTempDir` | Full `ProtoBufRecordReader` lifecycle (init, read, close) | All 5 tests confirmed to fail before the fix (each leaking 1 temp directory) and pass after. Full module suite: 172/172 tests pass. -- 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]
