You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "tarun11Mavani (via GitHub)" <gi...@apache.org> on 2026/06/19 13:49:12 UTC
[PR] fix(minion): fail single-segment task when segment upload fails [pinot]
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] fix(minion): fail single-segment task when segment upload fails [pinot]
Posted by "tarun11Mavani (via GitHub)" <gi...@apache.org>.
tarun11Mavani commented on code in PR #18813:
URL: https://github.com/apache/pinot/pull/18813#discussion_r3449898217
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/BaseSingleSegmentConversionExecutor.java:
##########
@@ -210,19 +209,17 @@ public SegmentConversionResult executeTask(PinotTaskConfig pinotTaskConfig)
throw new UnsupportedOperationException("Unrecognized push mode: " + pushType);
}
} catch (Exception e) {
- uploadSuccessful = false;
_minionMetrics.addMeteredTableValue(tableNameWithType, MinionMeter.SEGMENT_UPLOAD_FAIL_COUNT, 1L);
LOGGER.error("Segment upload failed for segment {}, table {}", segmentName, tableNameWithType, e);
_eventObserver.notifyTaskError(_pinotTaskConfig, e);
- }
- if (!FileUtils.deleteQuietly(convertedTarredSegmentFile)) {
- LOGGER.warn("Failed to delete tarred converted segment: {}", convertedTarredSegmentFile.getAbsolutePath());
- }
-
- if (uploadSuccessful) {
- LOGGER.info("Done executing {} on table: {}, segment: {}", taskType, tableNameWithType, segmentName);
+ throw e;
Review Comment:
Good catch — fixed in 9863fcc.
`uploadSegmentWithMetadata` now deletes the staged tar from the output PinotFS if the metadata send fails, before rethrowing, so a retry re-stages cleanly instead of hitting "Output file already exists".
I went with cleanup-on-failure rather than forcing `overwriteOutput=true`, so the existing guard against unrelated collisions in the output dir stays intact — we only remove the tar when it's our own stale partial from a failed attempt.
Added `testExecuteTaskCleansUpStagedTarWhenMetadataPushFails`, verified it fails without the cleanup.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] fix(minion): fail single-segment task when segment upload fails [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #18813:
URL: https://github.com/apache/pinot/pull/18813#discussion_r3446248750
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/BaseSingleSegmentConversionExecutor.java:
##########
@@ -210,19 +209,17 @@ public SegmentConversionResult executeTask(PinotTaskConfig pinotTaskConfig)
throw new UnsupportedOperationException("Unrecognized push mode: " + pushType);
}
} catch (Exception e) {
- uploadSuccessful = false;
_minionMetrics.addMeteredTableValue(tableNameWithType, MinionMeter.SEGMENT_UPLOAD_FAIL_COUNT, 1L);
LOGGER.error("Segment upload failed for segment {}, table {}", segmentName, tableNameWithType, e);
_eventObserver.notifyTaskError(_pinotTaskConfig, e);
- }
- if (!FileUtils.deleteQuietly(convertedTarredSegmentFile)) {
- LOGGER.warn("Failed to delete tarred converted segment: {}", convertedTarredSegmentFile.getAbsolutePath());
- }
-
- if (uploadSuccessful) {
- LOGGER.info("Done executing {} on table: {}, segment: {}", taskType, tableNameWithType, segmentName);
+ throw e;
Review Comment:
In METADATA push mode this rethrow turns controller-side push failures into task retries, but the retry comes back through `moveSegmentToOutputPinotFS()` with the same `<segment>.tar.gz` target. `overwriteOutput` is normally left false in `MinionTaskUtils.getPushTaskConfig()`, so if the first attempt already copied the tar before failing, the retry now dies on `Output file already exists` before it can resend metadata. That makes transient metadata-push failures sticky instead of self-healing. Can we make the staged tar idempotent across retries or clean it up before rethrowing?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] fix(minion): fail single-segment task when segment upload fails [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #18813:
URL: https://github.com/apache/pinot/pull/18813#issuecomment-4752523699
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/18813?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 56.99%. Comparing base ([`5526d5b`](https://app.codecov.io/gh/apache/pinot/commit/5526d5b86018bc8d22e10ece356d107dff5e280f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`bda2c72`](https://app.codecov.io/gh/apache/pinot/commit/bda2c72dc3928c57313d7bebf888c7a3fe94d272?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
> :exclamation: There is a different number of reports uploaded between BASE (5526d5b) and HEAD (bda2c72). Click for more details.
>
> <details><summary>HEAD has 16 uploads less than BASE</summary>
>
>| Flag | BASE (5526d5b) | HEAD (bda2c72) |
>|------|------|------|
>|java-21|5|1|
>|unittests|2|1|
>|temurin|5|1|
>|unittests2|1|0|
>|integration|3|0|
>|integration1|1|0|
>|integration2|1|0|
>|custom-integration1|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #18813 +/- ##
============================================
- Coverage 64.82% 56.99% -7.83%
+ Complexity 1319 1 -1318
============================================
Files 3388 2599 -789
Lines 210228 151683 -58545
Branches 32948 24567 -8381
============================================
- Hits 136282 86459 -49823
+ Misses 62978 57890 -5088
+ Partials 10968 7334 -3634
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/18813/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/18813/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/18813/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/18813/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/18813/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/18813/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.99% <ø> (-7.83%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/18813/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.99% <ø> (-7.83%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/18813/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.99% <ø> (-7.83%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/18813/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.99% <ø> (-0.01%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/18813/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/pinot/pull/18813?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] fix(minion): fail single-segment task when segment upload fails [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #18813:
URL: https://github.com/apache/pinot/pull/18813
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] fix(minion): fail single-segment task when segment upload fails [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #18813:
URL: https://github.com/apache/pinot/pull/18813#issuecomment-5037930015
Follow-up docs PR: https://github.com/pinot-contrib/pinot-docs/pull/933
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] fix(minion): fail single-segment task when segment upload fails [pinot]
Posted by "tarun11Mavani (via GitHub)" <gi...@apache.org>.
tarun11Mavani commented on PR #18813:
URL: https://github.com/apache/pinot/pull/18813#issuecomment-4829293096
@xiangfu0 addressed your comments. Can you please help merge this?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org