tarun11Mavani opened a new pull request, #18813:
URL: https://github.com/apache/pinot/pull/18813
## Summary
`BaseSingleSegmentConversionExecutor` caught segment-upload exceptions,
logged
and metered them, and then **returned the conversion result normally** — so
the
minion task was reported as SUCCESS even though the converted segment was
never
uploaded. Helix marked the task COMPLETED and never retried it, silently
leaving
the segment un-refreshed / un-purged / un-compacted.
This affects all single-segment conversion tasks:
`RealtimeToOfflineSegments`,
`PurgeTask`, `RefreshSegment`, `UpsertCompaction`, etc.
## Root cause
The swallow was an accidental control-flow change introduced in #10978
("Add minion observability for segment upload/download failures"). That PR
wrapped both the download and the upload calls to add metrics + logging:
- the **download** branch metered, logged, and **rethrew** the exception;
- the **upload** branch metered and logged but **omitted the rethrow**.
So the upload path has silently swallowed failures since June 2023. The
download-path asymmetry shows this was an oversight, not an intentional
"don't retry on upload failure" design — the PR's stated intent was
observability only.
## Change
- Rethrow the upload exception, mirroring the download path, so the task
fails
and is retried by the framework.
- Move the tarred-file cleanup into a `finally` block so it still runs on the
failure path (the file also lives under `tempDataDir`, which the outer
`finally` already deletes, so cleanup is preserved either way).
- Remove the now-dead `uploadSuccessful` flag.
## Testing
Added `BaseSingleSegmentConversionExecutorTest` (new file):
- `testExecuteTaskRethrowsWhenUploadFails` — static-mocks
`SegmentConversionUtils.uploadSegment` to throw and asserts `executeTask`
propagates the exception (the regression guard for this fix).
- `testExecuteTaskSucceedsWhenUploadSucceeds` — control test asserting the
success path returns the conversion result and the upload is invoked.
The test uses a test-only executor that stubs the download / CRC / convert /
ZK-metadata-modifier hooks so `executeTask` reaches the upload step without a
server, controller, or deep store, and restores the mutated process-global
state in teardown.
--
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]
