You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "jayeshchoudhary (via GitHub)" <gi...@apache.org> on 2026/03/06 13:44:48 UTC
[PR] Fix negative reload estimated time and year 2088 rebalance timestamp [pinot]
jayeshchoudhary opened a new pull request, #17833:
URL: https://github.com/apache/pinot/pull/17833
## Summary
Fixes two bugs in controller job status reporting:
- **Negative reload estimated time**: `estimatedTimeRemainingInMinutes` goes negative and stays there indefinitely when `successCount` exceeds `totalSegmentCount`. This happens when segments are added/replaced after the reload job was submitted, causing `totalSegmentCount - successCount` to go negative. Fixed by clamping `remainingSegments` to `Math.max(0, ...)`. Also added a derived `status` field (`COMPLETED` / `COMPLETED_WITH_ERRORS` / `IN_PROGRESS`) to the reload status response so consumers no longer have to infer job completion from raw counts.
- **Year 2088 rebalance "Finished at" timestamp**: `timeToFinishInSeconds` becomes a Unix timestamp (~1.7 billion) instead of an elapsed duration when `startTimeMs` is never initialized (default `0`). This occurs for `NO_OP` or early-failure rebalances that skip `START_TRIGGER`. The UI then computes `submissionTimeMs + (timestamp × 1000)` ≈ year 2080–2088. Fixed by guarding against `startTimeMs <= 0` and returning `0` seconds elapsed.
## Test plan
- [ ] Verify `pinot-controller` module compiles cleanly (`mvn compile -pl pinot-controller -am`)
- [ ] Existing `PinotTableReloadStatusReporterTest` passes
- [ ] Manual: trigger a table reload, confirm `estimatedTimeRemainingInMinutes >= 0` in the response
- [ ] Manual: confirm new `status` field appears in reload status JSON (`IN_PROGRESS` during reload, `COMPLETED` or `COMPLETED_WITH_ERRORS` after)
- [ ] Manual: trigger a NO_OP rebalance, confirm `timeToFinishInSeconds` is `0` (not a Unix timestamp) and UI shows a reasonable "Finished at" time
Made with [Cursor](https://cursor.com)
--
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 negative reload estimated time and year 2088 rebalance timestamp [pinot]
Posted by "jayeshchoudhary (via GitHub)" <gi...@apache.org>.
jayeshchoudhary commented on PR #17833:
URL: https://github.com/apache/pinot/pull/17833#issuecomment-4011843373
FYI: this is a AI generated PR, I am not a JAVA expert. Please help validate the fix and only then merge the changes
cc @xiangfu0
--
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 negative reload estimated time and year 2088 rebalance timestamp [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #17833:
URL: https://github.com/apache/pinot/pull/17833#discussion_r2896977065
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/ZkBasedTableRebalanceObserver.java:
##########
@@ -226,13 +224,25 @@ public void onSuccess(String msg) {
@Override
public void onError(String errorMsg) {
_controllerMetrics.setValueOfTableGauge(_tableNameWithType, ControllerGauge.TABLE_REBALANCE_IN_PROGRESS, 0);
- long timeToFinishInSeconds = (System.currentTimeMillis() - _tableRebalanceProgressStats.getStartTimeMs()) / 1000L;
- _tableRebalanceProgressStats.setTimeToFinishInSeconds(timeToFinishInSeconds);
+ _tableRebalanceProgressStats.setTimeToFinishInSeconds(computeElapsedTimeInSeconds());
_tableRebalanceProgressStats.setStatus(RebalanceResult.Status.FAILED);
_tableRebalanceProgressStats.setCompletionStatusMsg(errorMsg);
trackStatsInZk();
}
+ /**
+ * Safely computes elapsed time in seconds since rebalance started.
+ * Returns 0 if startTimeMs was never set (i.e. still at default 0), which happens
+ * when the rebalance completes as NO_OP or fails before START_TRIGGER fires.
+ */
+ private long computeElapsedTimeInSeconds() {
+ long startTimeMs = _tableRebalanceProgressStats.getStartTimeMs();
+ if (startTimeMs <= 0) {
+ return 0L;
+ }
+ return (System.currentTimeMillis() - startTimeMs) / 1000L;
+ }
Review Comment:
computeElapsedTimeInSeconds() introduces new behavior for NO_OP/early-failure cases (startTimeMs<=0 => 0 seconds). Please add/extend unit tests (e.g., in TestZkBasedTableRebalanceObserver) to cover onNoop/onError when startTimeMs was never initialized, so we don’t regress back to the “Unix timestamp” behavior.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/services/PinotTableReloadStatusReporter.java:
##########
@@ -80,6 +81,26 @@ private static double computeEstimatedRemainingTimeInMinutes(PinotTableReloadSta
return estimatedRemainingTimeInMinutes;
}
+ /**
+ * Derives the overall reload job status from aggregated counts.
+ * - COMPLETED: all segments reloaded successfully, no server call failures
+ * - COMPLETED_WITH_ERRORS: reload finished but some segments failed
+ * - IN_PROGRESS: reload is still running
+ */
+ private static String deriveReloadStatus(PinotTableReloadStatusResponse response) {
+ int processed = response.getSuccessCount() + (response.getFailureCount() != null
+ ? response.getFailureCount().intValue() : 0);
+ boolean allProcessed = processed >= response.getTotalSegmentCount();
+
+ if (allProcessed && response.getTotalServerCallsFailed() == 0) {
+ if (response.getFailureCount() != null && response.getFailureCount() > 0) {
+ return "COMPLETED_WITH_ERRORS";
+ }
+ return "COMPLETED";
+ }
+ return "IN_PROGRESS";
+ }
Review Comment:
New reload status derivation and the remaining-time clamping introduce new behavior (e.g., COMPLETED vs COMPLETED_WITH_ERRORS vs IN_PROGRESS, and successCount > totalSegmentCount handling) but there are no unit tests covering these branches. Please add tests in PinotTableReloadStatusReporterTest to lock in the expected status/ETA for: (1) all success, (2) failures present, (3) server-call failures, and (4) successCount > totalSegmentCount.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/services/PinotTableReloadStatusReporter.java:
##########
@@ -80,6 +81,26 @@ private static double computeEstimatedRemainingTimeInMinutes(PinotTableReloadSta
return estimatedRemainingTimeInMinutes;
}
+ /**
+ * Derives the overall reload job status from aggregated counts.
+ * - COMPLETED: all segments reloaded successfully, no server call failures
+ * - COMPLETED_WITH_ERRORS: reload finished but some segments failed
+ * - IN_PROGRESS: reload is still running
+ */
+ private static String deriveReloadStatus(PinotTableReloadStatusResponse response) {
+ int processed = response.getSuccessCount() + (response.getFailureCount() != null
+ ? response.getFailureCount().intValue() : 0);
+ boolean allProcessed = processed >= response.getTotalSegmentCount();
+
+ if (allProcessed && response.getTotalServerCallsFailed() == 0) {
+ if (response.getFailureCount() != null && response.getFailureCount() > 0) {
+ return "COMPLETED_WITH_ERRORS";
+ }
+ return "COMPLETED";
+ }
+ return "IN_PROGRESS";
Review Comment:
deriveReloadStatus() treats any non-zero totalServerCallsFailed as "IN_PROGRESS" even when all segments appear processed. If a server call fails (transiently or permanently), the reload job may be finished but this status would never become COMPLETED/COMPLETED_WITH_ERRORS, undermining the purpose of adding a status field. Consider returning COMPLETED_WITH_ERRORS when all segments are processed but there were server-call failures and/or segment failures (e.g., failureCount>0 or segmentReloadFailures non-empty).
```suggestion
* - COMPLETED: all segments reloaded successfully, no segment or server call failures
* - COMPLETED_WITH_ERRORS: reload finished but some segments and/or server calls failed
* - IN_PROGRESS: reload is still running (not all segments processed yet)
*/
private static String deriveReloadStatus(PinotTableReloadStatusResponse response) {
int failureCount = response.getFailureCount() != null ? response.getFailureCount() : 0;
int processed = response.getSuccessCount() + failureCount;
boolean allProcessed = processed >= response.getTotalSegmentCount();
// If not all segments have been accounted for, the reload is still in progress.
if (!allProcessed) {
return "IN_PROGRESS";
}
// Reload has finished (all segments processed). Determine if it completed cleanly or with errors.
boolean hasSegmentFailures = failureCount > 0;
boolean hasServerCallFailures = response.getTotalServerCallsFailed() > 0;
boolean hasSegmentReloadFailures = false;
if (response.getSegmentReloadFailures() != null && !response.getSegmentReloadFailures().isEmpty()) {
hasSegmentReloadFailures = true;
}
if (hasSegmentFailures || hasServerCallFailures || hasSegmentReloadFailures) {
return "COMPLETED_WITH_ERRORS";
}
return "COMPLETED";
```
--
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 negative reload estimated time and year 2088 rebalance timestamp [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #17833:
URL: https://github.com/apache/pinot/pull/17833#issuecomment-4012103858
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/17833?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 55.56%. Comparing base ([`fa8ad70`](https://app.codecov.io/gh/apache/pinot/commit/fa8ad7084a0135be8fde1623463c8563bafc3518?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`6b152a4`](https://app.codecov.io/gh/apache/pinot/commit/6b152a43bc26330ddf00721480ebf6817b5eb38d?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 (fa8ad70) and HEAD (6b152a4). Click for more details.
>
> <details><summary>HEAD has 36 uploads less than BASE</summary>
>
>| Flag | BASE (fa8ad70) | HEAD (6b152a4) |
>|------|------|------|
>|java-21|5|1|
>|unittests1|2|1|
>|unittests|4|1|
>|temurin|10|1|
>|java-11|5|0|
>|unittests2|2|0|
>|integration|6|0|
>|integration2|2|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #17833 +/- ##
============================================
- Coverage 63.25% 55.56% -7.69%
+ Complexity 1456 720 -736
============================================
Files 3189 2493 -696
Lines 191769 141123 -50646
Branches 29354 22505 -6849
============================================
- Hits 121297 78413 -42884
+ Misses 60981 56058 -4923
+ Partials 9491 6652 -2839
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/17833/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/17833/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/17833/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/17833/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/17833/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/17833/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/17833/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.56% <ø> (-7.67%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/17833/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.56% <ø> (-7.69%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/17833/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.56% <ø> (-7.69%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/17833/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.56% <ø> (-0.01%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/17833/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/17833?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 negative reload estimated time and year 2088 rebalance timestamp [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #17833:
URL: https://github.com/apache/pinot/pull/17833
--
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