You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "xiangfu0 (via GitHub)" <gi...@apache.org> on 2025/09/14 17:51:29 UTC
[PR] Re-ingest COMMITTING LLC segments when pauseless is disabled [pinot]
xiangfu0 opened a new pull request, #16811:
URL: https://github.com/apache/pinot/pull/16811
Controller: re-ingest COMMITTING LLC segments when pauseless is disabled.\n\n- Add PinotLLCRealtimeSegmentManager#reingestCommittingSegmentsForPauselessDisabled() and wire from RealtimeSegmentValidationManager when pauseless=false\n- Target only COMMITTING LLC segments with start/end offsets and no download URL that are ONLINE in IS\n- Respect partial upsert/dedup safety via allowRepairOfErrorSegments\n- Add unit tests in RealtimeSegmentValidationManagerTest\n\nBuild: checkstyle clean, targeted unit test passing.
--
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] Re-ingest COMMITTING LLC segments when pauseless is disabled [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #16811:
URL: https://github.com/apache/pinot/pull/16811#discussion_r2365159103
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -2709,16 +2708,15 @@ public void repairSegmentsInErrorStateForPauselessConsumption(TableConfig tableC
assert idealStateMap != null;
String aliveServer = pickServerToReingest(idealStateMap.keySet());
if (aliveServer == null) {
- LOGGER.warn("No alive server found to re-ingest segment: {} in table: {}, skipping re-ingestion", segmentName,
- realtimeTableName);
- continue;
+ LOGGER.warn("No alive server found to re-ingest segment: {} in table: {}", segmentName, realtimeTableName);
+ return;
Review Comment:
good catch
--
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] Re-ingest COMMITTING LLC segments when pauseless is disabled [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #16811:
URL: https://github.com/apache/pinot/pull/16811#discussion_r2365157819
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -2709,16 +2708,15 @@ public void repairSegmentsInErrorStateForPauselessConsumption(TableConfig tableC
assert idealStateMap != null;
String aliveServer = pickServerToReingest(idealStateMap.keySet());
if (aliveServer == null) {
- LOGGER.warn("No alive server found to re-ingest segment: {} in table: {}, skipping re-ingestion", segmentName,
- realtimeTableName);
- continue;
+ LOGGER.warn("No alive server found to re-ingest segment: {} in table: {}", segmentName, realtimeTableName);
+ return;
Review Comment:
Changing from `continue` to `return` will exit the entire method when no alive server is found for any segment, preventing re-ingestion attempts for remaining segments. This should remain as `continue` to skip only the current segment and process others.
```suggestion
continue;
```
--
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] Re-ingest COMMITTING LLC segments when pauseless is disabled [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #16811:
URL: https://github.com/apache/pinot/pull/16811#issuecomment-3289764242
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/16811?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.28%. Comparing base ([`c897df7`](https://app.codecov.io/gh/apache/pinot/commit/c897df7c89435c78d2f795ba7defdcaa9dfeabc1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`487b58f`](https://app.codecov.io/gh/apache/pinot/commit/487b58fdaa9005695321b4e4de9348ec0e816db9?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 (c897df7) and HEAD (487b58f). Click for more details.
>
> <details><summary>HEAD has 36 uploads less than BASE</summary>
>
>| Flag | BASE (c897df7) | HEAD (487b58f) |
>|------|------|------|
>|java-21|5|1|
>|unittests1|2|1|
>|unittests|4|1|
>|temurin|10|1|
>|java-11|5|0|
>|unittests2|2|0|
>|integration|6|0|
>|integration1|2|0|
>|custom-integration1|2|0|
>|integration2|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #16811 +/- ##
============================================
- Coverage 63.38% 56.28% -7.11%
+ Complexity 1399 700 -699
============================================
Files 3055 2396 -659
Lines 178934 133178 -45756
Branches 27423 21189 -6234
============================================
- Hits 113417 74956 -38461
+ Misses 56775 51972 -4803
+ Partials 8742 6250 -2492
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/16811/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/16811/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/16811/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/16811/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/16811/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/16811/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/16811/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.28% <ø> (-7.06%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/16811/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.28% <ø> (-7.11%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/16811/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.28% <ø> (-7.10%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/16811/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.28% <ø> (-0.06%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/16811/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 Sentry](https://app.codecov.io/gh/apache/pinot/pull/16811?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] Re-ingest COMMITTING LLC segments when pauseless is disabled [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #16811:
URL: https://github.com/apache/pinot/pull/16811#issuecomment-3610455192
> Trying to understand the scenario this PR tries to fix: pauseless was initially enabled, then disabled while some segments are left in committing state?
Yes, we saw an issue during turning on the pauseless on a table. After turning off pauseless, we found one segment stuck in the `COMMITTING` status for each partition.
There is no way to fix, unless we delete all the segments from this `COMMITTING` segments to the CONSUMING segments.
--
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] Re-ingest COMMITTING LLC segments when pauseless is disabled [pinot]
Posted by "noob-se7en (via GitHub)" <gi...@apache.org>.
noob-se7en commented on code in PR #16811:
URL: https://github.com/apache/pinot/pull/16811#discussion_r2348356712
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -2689,18 +2689,8 @@ public void repairSegmentsInErrorStateForPauselessConsumption(TableConfig tableC
// Find at least one server that should host this segment and is alive
Map<String, String> idealStateMap = segmentToInstanceIdealStateMap.get(segmentName);
assert idealStateMap != null;
- String aliveServer = pickServerToReingest(idealStateMap.keySet());
- if (aliveServer == null) {
- LOGGER.warn("No alive server found to re-ingest segment: {} in table: {}, skipping re-ingestion", segmentName,
- realtimeTableName);
- continue;
- }
-
- try {
- triggerReingestion(aliveServer, segmentName);
- LOGGER.info("Successfully triggered re-ingestion for segment: {} on server: {}", segmentName, aliveServer);
- } catch (Exception e) {
- LOGGER.error("Failed to call reingestSegment for segment: {} on server: {}", segmentName, aliveServer, e);
+ if (hasOnlineInstance(idealStateMap)) {
Review Comment:
This is already checked above I beleive?
--
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] Re-ingest COMMITTING LLC segments when pauseless is disabled [pinot]
Posted by "noob-se7en (via GitHub)" <gi...@apache.org>.
noob-se7en commented on code in PR #16811:
URL: https://github.com/apache/pinot/pull/16811#discussion_r2355004362
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -2709,16 +2708,15 @@ public void repairSegmentsInErrorStateForPauselessConsumption(TableConfig tableC
assert idealStateMap != null;
String aliveServer = pickServerToReingest(idealStateMap.keySet());
if (aliveServer == null) {
- LOGGER.warn("No alive server found to re-ingest segment: {} in table: {}, skipping re-ingestion", segmentName,
- realtimeTableName);
- continue;
+ LOGGER.warn("No alive server found to re-ingest segment: {} in table: {}", segmentName, realtimeTableName);
+ return;
Review Comment:
why return?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/validation/RealtimeSegmentValidationManager.java:
##########
@@ -130,14 +130,13 @@ protected void processTable(String tableNameWithType, Context context) {
LOGGER.info("Skipping segment-level validation for table: {}", tableConfig.getTableName());
}
- boolean isPauselessConsumptionEnabled = PauselessConsumptionUtils.isPauselessEnabled(tableConfig);
- if (isPauselessConsumptionEnabled) {
- // For pauseless tables without dedup or partial upsert, repair segments in error state
- _llcRealtimeSegmentManager.repairSegmentsInErrorStateForPauselessConsumption(tableConfig,
- context._repairErrorSegmentsForPartialUpsertOrDedup);
- } else if (_segmentAutoResetOnErrorAtValidation) {
- // Reset for pauseless tables is already handled in repairSegmentsInErrorStateForPauselessConsumption method with
- // additional checks for pauseless consumption
+ // For realtime tables without dedup or partial upsert, repair segments in error state
+ // This used to only work for pauseless table, but now extends the usage to all tables
+ _llcRealtimeSegmentManager.repairSegmentsInErrorStateForPauselessConsumption(tableConfig,
+ context._repairErrorSegmentsForPartialUpsertOrDedup);
+ if (_segmentAutoResetOnErrorAtValidation) {
Review Comment:
Isn't this a behaviour change causing reset of segment twice?
--
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] Re-ingest COMMITTING LLC segments when pauseless is disabled [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #16811:
URL: https://github.com/apache/pinot/pull/16811#discussion_r2365692486
##########
pinot-controller/src/main/java/org/apache/pinot/controller/validation/RealtimeSegmentValidationManager.java:
##########
@@ -130,14 +130,13 @@ protected void processTable(String tableNameWithType, Context context) {
LOGGER.info("Skipping segment-level validation for table: {}", tableConfig.getTableName());
}
- boolean isPauselessConsumptionEnabled = PauselessConsumptionUtils.isPauselessEnabled(tableConfig);
- if (isPauselessConsumptionEnabled) {
- // For pauseless tables without dedup or partial upsert, repair segments in error state
- _llcRealtimeSegmentManager.repairSegmentsInErrorStateForPauselessConsumption(tableConfig,
- context._repairErrorSegmentsForPartialUpsertOrDedup);
- } else if (_segmentAutoResetOnErrorAtValidation) {
- // Reset for pauseless tables is already handled in repairSegmentsInErrorStateForPauselessConsumption method with
- // additional checks for pauseless consumption
+ // For realtime tables without dedup or partial upsert, repair segments in error state
+ // This used to only work for pauseless table, but now extends the usage to all tables
+ _llcRealtimeSegmentManager.repairSegmentsInErrorStateForPauselessConsumption(tableConfig,
+ context._repairErrorSegmentsForPartialUpsertOrDedup);
+ if (_segmentAutoResetOnErrorAtValidation) {
Review Comment:
good point, repurpose this `repairSegmentsInErrorStateForPauselessConsumption` to `repairSegmentsInErrorState` to fix all realtime segment error state
--
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] Re-ingest COMMITTING LLC segments when pauseless is disabled [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #16811:
URL: https://github.com/apache/pinot/pull/16811#discussion_r2365159103
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -2709,16 +2708,15 @@ public void repairSegmentsInErrorStateForPauselessConsumption(TableConfig tableC
assert idealStateMap != null;
String aliveServer = pickServerToReingest(idealStateMap.keySet());
if (aliveServer == null) {
- LOGGER.warn("No alive server found to re-ingest segment: {} in table: {}, skipping re-ingestion", segmentName,
- realtimeTableName);
- continue;
+ LOGGER.warn("No alive server found to re-ingest segment: {} in table: {}", segmentName, realtimeTableName);
+ return;
Review Comment:
good catch, should be continue
--
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] Re-ingest COMMITTING LLC segments when pauseless is disabled [pinot]
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #16811:
URL: https://github.com/apache/pinot/pull/16811#discussion_r2373083937
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -2717,16 +2727,15 @@ public void repairSegmentsInErrorStateForPauselessConsumption(TableConfig tableC
assert idealStateMap != null;
String aliveServer = pickServerToReingest(idealStateMap.keySet());
if (aliveServer == null) {
- LOGGER.warn("No alive server found to re-ingest segment: {} in table: {}, skipping re-ingestion", segmentName,
- realtimeTableName);
+ LOGGER.warn("No alive server found to re-ingest segment: {} in table: {}", segmentName, realtimeTableName);
Review Comment:
nit: was there some reason to remove ", skipping re-ingestion"? seems to be easier to understand the log if this is included, right?
--
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] Re-ingest COMMITTING LLC segments when pauseless is disabled [pinot]
Posted by "noob-se7en (via GitHub)" <gi...@apache.org>.
noob-se7en commented on PR #16811:
URL: https://github.com/apache/pinot/pull/16811#issuecomment-3346064174
I have fixed some bugs in method: `repairSegmentsInErrorStateForPauselessConsumption` and added tests
https://github.com/apache/pinot/pull/16904
Will request to wait before merging this PR.
--
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] Re-ingest COMMITTING LLC segments when pauseless is disabled [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #16811:
URL: https://github.com/apache/pinot/pull/16811#issuecomment-3544256174
Trying to understand the scenario this PR tries to fix: pauseless was initially enabled, then disabled while some segments are left in committing state?
--
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] Re-ingest COMMITTING LLC segments when pauseless is disabled [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #16811:
URL: https://github.com/apache/pinot/pull/16811
--
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