shounakmk219 opened a new pull request, #19627: URL: https://github.com/apache/pinot/pull/19627
## Problem Segment uploads leave segment-sized files behind in `java.io.tmpdir` when a request fails, and no change to the upload resource can prevent it — the files are orphaned during Jersey's parse phase, before the resource method is entered. `MultiPartReaderServerSide` registers the parsed `MultiPart` with the request's `CloseableService` only **after** parsing succeeds: ```java MultiPart mp = super.readMultiPart(...); closeableServiceProvider.get().add(mp); // never reached if the line above throws return mp; ``` and `MultiPartReaderClientSide.readMultiPart` has no `try`/`finally` around the `MIMEMessage` — its exception table holds a single entry, for the `IllegalArgumentException` around `getContentDisposition()`. So any parse failure abandons whatever has already been spilled to disk. Three paths reach it: | Path | Trigger | |---|---| | `MIMEParsingException` from `getMimeParts()` | truncated upload, client disconnect, bad boundary | | `ClientErrorException(413)` on `maxParts` | thrown *after* every part has already been buffered | | `IllegalArgumentException` → `BadRequestException` | malformed `Content-Disposition` | mimepull does eventually reclaim these via its weak-reference queue (`WeakDataFile` + `drainRefQueueBounded`), but only once the `DataFile` is collected **and** a later multipart request drains the queue. After a burst of failed uploads the files just sit there, and if uploads stop nothing drains the queue at all. Severity comes from where they land: `ControllerAdminApiApplication` registered `MultiPartFeature` with no `MultiPartProperties`, so mimepull's defaults applied — a 1 MB memory threshold (every segment tar is far over it, so the whole segment spills) and `java.io.tmpdir` as the directory, which nothing sweeps. ## Fix **Point Jersey at a controller-owned directory.** `ControllerFilePathProvider` now manages a `multipartTemp` directory alongside `fileUploadTemp` / `untarredFileTemp` / `fileDownloadTemp`, created through the same `initDir` and therefore cleared on startup. `ControllerAdminApiApplication` registers a `ContextResolver<MultiPartProperties>` pointing at it. Orphaned parts are reclaimed on restart instead of accumulating for the life of the host, and they sit on the volume operators already size for uploads. The resolver resolves the directory **lazily**: the admin application is constructed at `BaseControllerStarter#297` but `initControllerFilePathProvider()` does not run until `#575`. Jersey does not build the multipart reader until the first multipart request, well after startup. If resolution ever throws it falls back to the JVM default rather than failing the request, matching Jersey's own behaviour when a configured directory turns out to be unusable. **Release the multipart eagerly.** `uploadSegment` and `uploadReingestedSegment` now call cleanup in their `finally` blocks, as `uploadSegments` already did. This is explicitly *not* a leak fix — `CloseableService` closes a successfully-parsed multipart at end of request regardless — but it stops the controller from holding a second full copy of the segment on disk across the ZK and deep-store operations that follow. It runs through a guarded helper that is null-safe on the URI upload path (where `multiPart` is `@Nullable`) and swallows exceptions so cleanup can never replace the exception that led into the `finally`. ## Testing Three new tests in `ControllerAdminApiApplicationTest`: - `testJerseyResolvesTheRegisteredMultiPartProperties` builds a real `ApplicationHandler` and performs the exact `providers.getContextResolver(MultiPartProperties.class, WILDCARD_TYPE)` lookup that `MultiPartReaderClientSide` does when constructing its `MIMEConfig`. This is the load-bearing one: a `MultiPartProperties` set as an application *property* would have been silently ignored, so proving the resolver is actually reachable through `Providers` matters. - `testMultiPartTempDirResolvesToControllerTempDir` pins the resolved path. - `testAdminApplicationRegistersTheMultiPartTempDirResolver` guards the registration itself — without it the other two still pass while Jersey quietly keeps using `java.io.tmpdir`. Plus `testStaleMultiPartFilesClearedOnInit` in `ControllerFilePathProviderTest` (the startup clean is the behaviour the fix depends on), the existing three provider tests extended to cover the new directory, and two tests for the cleanup helper's null-safety and exception containment. 21 tests pass across the three classes; checkstyle and license are clean. Confirmed the tests are not vacuous by reverting each change and re-running: - removing the `register(...)` line → `testAdminApplicationRegistersTheMultiPartTempDirResolver` fails `expected [true] but found [false]` - downgrading `initDir` to `forceMkdir` → `testStaleMultiPartFilesClearedOnInit` fails `expected [false] but found [true]` ```bash mvn -pl pinot-controller test -Dtest='ControllerAdminApiApplicationTest,ControllerFilePathProviderTest,PinotSegmentUploadDownloadRestletResourceTest' ``` ## Notes This contains the problem rather than fixing it at the root. `MultiPartReaderClientSide.readMultiPart` still abandons parts on a parse failure — they are simply abandoned somewhere the controller sweeps. The upstream fix would be to close the `MIMEMessage` when parsing throws. **Operator-visible change:** multipart spill moves from `java.io.tmpdir` onto whatever volume backs `controller.local.temp.dir`. Deployments where those are separate mounts will see write volume shift accordingly. ## Backward compatibility No wire format, serialization, or public API change. `MultiPartTempDirResolver` is package-private; `getMultiPartTempDir()` is additive. -- 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]
