shounakmk219 opened a new pull request, #19377: URL: https://github.com/apache/pinot/pull/19377
## Problem The batch metadata segment upload path (`POST /segments/batchUpload`) creates three temporary files under `java.io.tmpdir` that are never deleted: | File | Created in | Scope | |---|---|---| | `segmentMetadata-<uuid>/` | `createSegmentFileFromSegmentMetadataInfo` | per segment | | `allSegmentsMetadataTar-<uuid>.tar.gz` | `createSegmentsMetadataInfoMap` | per request | | `allSegmentsMetadataDir-<uuid>/` | `createSegmentsMetadataInfoMap` | per request | `uploadSegments` already has a `tempFiles` list drained by `cleanupTempFiles` in a `finally`, but it only ever receives the three per-segment entries (`tempEncryptedFile`, `tempDecryptedFile`, `tempSegmentDir`) which live under the controller's own `getFileUploadTempDir()` / `getUntarredFileTempDir()`. The three files above live in `java.io.tmpdir` instead, so `ControllerFilePathProvider.initDir`'s startup `cleanDirectory` does not reclaim them either — they survive controller restarts and are only removed when the OS reaps `/tmp`. In `createSegmentFileFromSegmentMetadataInfo`, only the intermediate tar file had a `finally`; the staging directory it was built from had none. Per push of N segments this leaves behind 1 tar + 1 directory holding 2N metadata files + N directories holding 2 files each — roughly 3x the metadata volume of every push. `metadata.properties` scales with column count, so wide tables on a frequent push cadence will fill `/tmp` on the controller. ## Fix - **`createSegmentFileFromSegmentMetadataInfo`** — open the `try` before the staging work so the `finally` covers the staging directory as well as the tar file. This also handles a `createCompressedTarFile` failure leaving a partial tar behind. Switched the `finally` from `forceDelete` to `deleteQuietly` so a cleanup failure cannot mask the exception that caused it. - **`createSegmentsMetadataInfoMap`** — takes the caller's `tempFiles` list and registers both request-scoped files as soon as their paths are computed, so they are cleaned even when the untar or the mapping-file read fails part way through. They are registered rather than deleted locally because the returned `SegmentMetadataInfo` values hold live `File` handles into that directory, which the caller reads inside its loop. - **`uploadSegments`** — moved the `createSegmentsMetadataInfoMap` call inside the `try`. Previously a failure there leaked both files *and* skipped `multiPart.cleanup()` entirely. `ControllerApplicationException` extends `WebApplicationException`, so the catch block rethrows it unchanged; the only behavioral delta is a metric increment of 0 and the cleanup now running. The equivalent client-side code in `SegmentPushUtils.createSegmentsMetadataTarFile` already deletes its staging directory in a `finally` — the controller side had not been given the same treatment. ## Testing - `testCreateSegmentFileFromSegmentMetadataInfo` extended with a before/after snapshot of `java.io.tmpdir` entries matching the two `segmentMetadata*` prefixes. - New `testCreateSegmentsMetadataInfoMapRegistersTempFilesForCleanup` builds a real uber tar and asserts the map is correct, that its file handles are still readable (cleanup correctly deferred to the caller), that both temp files landed in `tempFiles`, and that draining the list leaves no residue. Both assert on a prefix-filtered diff of the temp directory rather than absolute emptiness, so unrelated entries cannot flake them. Confirmed the tests are not vacuous — with the cleanup removed they fail with `expected [] but got [segmentMetadata-3cff66bd-…]` and `expected [2] but found [0]` respectively. `createSegmentsMetadataInfoMap` was made package-private `@VisibleForTesting` to allow the new test; it has no callers outside this class. ## Backward compatibility None affected — no wire format, serialization, or public API change. `createSegmentsMetadataInfoMap` is an internal static helper. -- 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]
