You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Jackie-Jiang (via GitHub)" <gi...@apache.org> on 2026/10/15 20:33:34 UTC
[PR] Simplify re-ingestion tracking and skip obsolete queued jobs [pinot]
Jackie-Jiang opened a new pull request, #19893:
URL: https://github.com/apache/pinot/pull/19893
Follow-up to #19737 addressing the remaining review suggestions.
Queued repairs previously retained metadata from the request, so a segment deleted or repaired by another replica could still start re-ingestion later. Re-resolve the table manager and fetch current segment metadata when the worker starts, skipping unavailable tables and segments that are missing, no longer COMMITTING, or already have a download URL. Skipped jobs are removed without incrementing the failure metric. This rechecks queued work; it does not coordinate repairs already running on different replicas.
- Use one segment-keyed job map for duplicate prevention and job listing, with conditional removal on completion or rejected submission.
- Remove the injection-only API test and consolidate overlapping timeout tests while retaining cluster-only configuration coverage.
- Add queued-job regression cases for deleted, completed, non-committing, and already-uploaded segments.
Validation: all 30 tests in `ReingestionResourceApiTest`, `ReingestionResourceTest`, and `ReingestionConsumptionTimeoutTest` passed using the `pinot-server` reactor build. Spotless, Checkstyle, license formatting, and license checks passed. Compilation ran with deprecation and warning reporting enabled.
--
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] Simplify re-ingestion tracking and skip obsolete queued jobs [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #19893:
URL: https://github.com/apache/pinot/pull/19893#discussion_r4288267773
##########
pinot-server/src/main/java/org/apache/pinot/server/api/resources/ReingestionResource.java:
##########
@@ -249,14 +232,26 @@ public void shutDown() {
LOGGER.info("Stopped re-ingestion executor, dropped {} jobs waiting for a re-ingestion thread", numDroppedJobs);
}
- /// The actual re-ingestion logic, moved into a separate method for clarity.
- /// This is essentially the old synchronous logic you had in reingestSegment.
- private void doReingestSegment(String realtimeTableName, SegmentZKMetadata segmentZKMetadata,
- IndexLoadingConfig indexLoadingConfig, @Nullable Semaphore segmentBuildSemaphore)
+ private void doReingestSegment(String realtimeTableName, String segmentName)
throws Exception {
- String segmentName = segmentZKMetadata.getSegmentName();
+ // The table or segment may have been removed or repaired on another replica while this job was queued.
+ RealtimeTableDataManager tableDataManager =
+ (RealtimeTableDataManager) _serverInstance.getInstanceDataManager().getTableDataManager(realtimeTableName);
+ if (tableDataManager == null || tableDataManager.isShutDown()) {
+ LOGGER.info("Skipping re-ingestion of segment: {} because table: {} is no longer available", segmentName,
+ realtimeTableName);
+ return;
+ }
+ SegmentZKMetadata segmentZKMetadata = tableDataManager.fetchZKMetadataNullable(segmentName);
+ if (segmentZKMetadata == null
+ || segmentZKMetadata.getStatus() != Status.COMMITTING
+ || segmentZKMetadata.getDownloadUrl() != null) {
+ LOGGER.info("Skipping re-ingestion of segment: {} because it was deleted or no longer needs repair", segmentName);
Review Comment:
MINOR (nit): The single log message covers three different skip reasons (deleted, not COMMITTING, already has a download URL). For easier debugging of why a queued job was dropped, you might log which of the three conditions actually matched.
##########
pinot-server/src/main/java/org/apache/pinot/server/api/resources/ReingestionResource.java:
##########
@@ -249,14 +232,26 @@ public void shutDown() {
LOGGER.info("Stopped re-ingestion executor, dropped {} jobs waiting for a re-ingestion thread", numDroppedJobs);
}
- /// The actual re-ingestion logic, moved into a separate method for clarity.
- /// This is essentially the old synchronous logic you had in reingestSegment.
- private void doReingestSegment(String realtimeTableName, SegmentZKMetadata segmentZKMetadata,
- IndexLoadingConfig indexLoadingConfig, @Nullable Semaphore segmentBuildSemaphore)
+ private void doReingestSegment(String realtimeTableName, String segmentName)
throws Exception {
- String segmentName = segmentZKMetadata.getSegmentName();
+ // The table or segment may have been removed or repaired on another replica while this job was queued.
+ RealtimeTableDataManager tableDataManager =
+ (RealtimeTableDataManager) _serverInstance.getInstanceDataManager().getTableDataManager(realtimeTableName);
+ if (tableDataManager == null || tableDataManager.isShutDown()) {
+ LOGGER.info("Skipping re-ingestion of segment: {} because table: {} is no longer available", segmentName,
+ realtimeTableName);
+ return;
Review Comment:
MINOR (coverage): This `tableDataManager == null || isShutDown()` skip branch isn't exercised by the new test. The data provider covers the stale-metadata cases but not the table-removed / shutdown case, so a regression here would go unnoticed. Consider adding a case that drops or shuts down the manager while the job is queued.
--
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] Simplify re-ingestion tracking and skip obsolete queued jobs [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #19893:
URL: https://github.com/apache/pinot/pull/19893#discussion_r4288404757
##########
pinot-server/src/main/java/org/apache/pinot/server/api/resources/ReingestionResource.java:
##########
@@ -249,14 +232,26 @@ public void shutDown() {
LOGGER.info("Stopped re-ingestion executor, dropped {} jobs waiting for a re-ingestion thread", numDroppedJobs);
}
- /// The actual re-ingestion logic, moved into a separate method for clarity.
- /// This is essentially the old synchronous logic you had in reingestSegment.
- private void doReingestSegment(String realtimeTableName, SegmentZKMetadata segmentZKMetadata,
- IndexLoadingConfig indexLoadingConfig, @Nullable Semaphore segmentBuildSemaphore)
+ private void doReingestSegment(String realtimeTableName, String segmentName)
throws Exception {
- String segmentName = segmentZKMetadata.getSegmentName();
+ // The table or segment may have been removed or repaired on another replica while this job was queued.
+ RealtimeTableDataManager tableDataManager =
+ (RealtimeTableDataManager) _serverInstance.getInstanceDataManager().getTableDataManager(realtimeTableName);
+ if (tableDataManager == null || tableDataManager.isShutDown()) {
+ LOGGER.info("Skipping re-ingestion of segment: {} because table: {} is no longer available", segmentName,
+ realtimeTableName);
+ return;
Review Comment:
Added queued-job cases for both a removed table manager and a shut-down manager in 1b01ccaa32. They verify that the worker skips metadata/config loading and writer setup, removes the job, and does not increment the failure metric. All 32 targeted tests pass.
##########
pinot-server/src/main/java/org/apache/pinot/server/api/resources/ReingestionResource.java:
##########
@@ -249,14 +232,26 @@ public void shutDown() {
LOGGER.info("Stopped re-ingestion executor, dropped {} jobs waiting for a re-ingestion thread", numDroppedJobs);
}
- /// The actual re-ingestion logic, moved into a separate method for clarity.
- /// This is essentially the old synchronous logic you had in reingestSegment.
- private void doReingestSegment(String realtimeTableName, SegmentZKMetadata segmentZKMetadata,
- IndexLoadingConfig indexLoadingConfig, @Nullable Semaphore segmentBuildSemaphore)
+ private void doReingestSegment(String realtimeTableName, String segmentName)
throws Exception {
- String segmentName = segmentZKMetadata.getSegmentName();
+ // The table or segment may have been removed or repaired on another replica while this job was queued.
+ RealtimeTableDataManager tableDataManager =
+ (RealtimeTableDataManager) _serverInstance.getInstanceDataManager().getTableDataManager(realtimeTableName);
+ if (tableDataManager == null || tableDataManager.isShutDown()) {
+ LOGGER.info("Skipping re-ingestion of segment: {} because table: {} is no longer available", segmentName,
+ realtimeTableName);
+ return;
+ }
+ SegmentZKMetadata segmentZKMetadata = tableDataManager.fetchZKMetadataNullable(segmentName);
+ if (segmentZKMetadata == null
+ || segmentZKMetadata.getStatus() != Status.COMMITTING
+ || segmentZKMetadata.getDownloadUrl() != null) {
+ LOGGER.info("Skipping re-ingestion of segment: {} because it was deleted or no longer needs repair", segmentName);
Review Comment:
Split the skip guards and added a distinct WARN message for each reason in 1b01ccaa32, including the actual status for non-COMMITTING segments. Skips still return normally without incrementing the failure metric.
--
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] Simplify re-ingestion tracking and skip obsolete queued jobs [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19893:
URL: https://github.com/apache/pinot/pull/19893#issuecomment-6203824461
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19893?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 100.00%. Comparing base ([`c15cead`](https://app.codecov.io/gh/apache/pinot/commit/c15ceadb2d1e56712686e8124178f7831cb83ac4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`1b01cca`](https://app.codecov.io/gh/apache/pinot/commit/1b01ccaa320563878f21fce9297d94cceeeba2b6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 9 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19893 +/- ##
==============================================
+ Coverage 69.26% 100.00% +30.73%
+ Complexity 1491 6 -1485
==============================================
Files 3543 3 -3540
Lines 233237 6 -233231
Branches 37277 0 -37277
==============================================
- Hits 161560 6 -161554
+ Misses 59184 0 -59184
+ Partials 12493 0 -12493
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19893/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/19893/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/19893/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/19893/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/19893/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (+30.73%)` | :arrow_up: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19893/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [lane-b](https://app.codecov.io/gh/apache/pinot/pull/19893/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/19893/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (+30.73%)` | :arrow_up: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19893/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/19893/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/19893/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/19893?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] Simplify re-ingestion tracking and skip obsolete queued jobs [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #19893:
URL: https://github.com/apache/pinot/pull/19893
--
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] Simplify re-ingestion tracking and skip obsolete queued jobs [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #19893:
URL: https://github.com/apache/pinot/pull/19893#issuecomment-6223469570
Documentation follow-up merged: https://github.com/pinot-contrib/pinot-docs/pull/1116 — explains worker-time obsolete re-ingestion job checks, skip logging/failure-meter behavior, and why an accepted job disappearing from the active list does not prove a rebuild.
--
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