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 2026/06/11 23:33:52 UTC
[PR] Fix table rebalance to not drop the last serving replicas when proceeding on best-efforts without ExternalView convergence [pinot]
xiangfu0 opened a new pull request, #18742:
URL: https://github.com/apache/pinot/pull/18742
## Problem
`TableRebalancer` enforces `minAvailableReplicas` against the **IdealState only**: when computing the next assignment, an instance counts as "available" for a segment merely because it appears in both the current and next IdealState (`SingleSegmentAssignment._availableInstances`). This is sound only because each incremental step waits for the ExternalView to converge before the next step — which is exactly the assumption that `bestEfforts=true` breaks: when the EV stabilization wait times out without progress, the rebalancer proceeds to the next step anyway.
In that situation, the rebalancer can remove the **last replica actually serving a segment** from the IdealState while the new replicas are still loading (or their servers are down), leaving the segment ONLINE nowhere.
This caused a production incident with tiered storage: during a cold-tier server pool restart (~3.5h reload), the hourly `SegmentRelocator` (which uses `bestEfforts=true` by default, with the EV stabilization timeout capped at the task interval) kept relocating segments hot → cold. Each run timed out waiting for the down replica group, force-advanced the IdealState, and dropped the hot-tier replicas while no cold server had loaded the segments. Under `strictReplicaGroup` routing the brokers then reported the **entire cold tier (~29.5k segments) unavailable** for ~1 minute every hour:
```
29562 segments unavailable, sampling 10: [...], with routing policy: strictReplicaGroup [realtime]
```
## Fix
When the EV stabilization wait gives up without convergence (best-efforts), the wait now returns the last-seen ExternalView, and the next assignment calculation uses it to keep `minAvailableReplicas` in terms of replicas **actually serving** each segment (ONLINE/CONSUMING in EV):
- A segment is not moved if the move would drop its serving replicas below `min(minAvailableReplicas, numServingReplicas)`. Capping at `numServingReplicas` means already-degraded segments (ERROR everywhere, hosted on dead instances, or absent from EV) never block the rebalance — moving them cannot reduce serving replicas below what they already are, so best-efforts still powers through ERROR states and dead-server evacuation.
- For strict replica group assignment, the check is enforced per `(currentInstances, targetInstances)` group, not per segment: all segments sharing the same instance pair (in particular all segments of a partition) are held together, so the IdealState never splits a partition across instance sets (which would break strict replica group routing consistency for upsert tables).
- If **no** segment can be moved safely, the rebalance now returns `FAILED` instead of looping or silently causing downtime. It can be retried (e.g. the next `SegmentRelocator` run) once the serving instances recover.
- When the EV converges normally (the common case), behavior is **completely unchanged** — the EV is not consulted at all.
## bestEfforts semantic change
Previously, `bestEfforts=true` + non-converged EV meant "continue to the next stage" unconditionally — including dropping the last serving replicas (query downtime, silently). Now it means "continue for the segments that can be moved without dropping their serving replicas below the minimum; fail if none can". The `RebalanceConfig` comment and the REST `@ApiParam` doc are updated accordingly. ERROR-state handling is unchanged (still counted as good state / powered through).
## Testing
- `TableRebalancerTest.testNextAssignmentWithNonConvergedExternalView`: unit test of the gating for strict and non-strict replica group, batched and non-batched, covering: hold when target replicas not loaded; per-segment moves for non-strict vs whole-group moves for strict when partially loaded; progress when loaded; no blocking for ERROR-everywhere / EV-absent segments; CONSUMING counts as serving; `minAvailableReplicas=0` (downtime) unaffected.
- `TableRebalancerClusterStatelessTest.testBestEffortsRebalanceDoesNotDropServingReplicas`: end-to-end reproduction of the incident shape — moving all segments to a disjoint set of servers whose participants are down; asserts the rebalance returns `FAILED` with every segment still having a serving replica in the IdealState, then completes with `DONE` once the target servers come back.
- Full `TableRebalancerTest`, `TableRebalancerClusterStatelessTest`, `RebalanceCheckerTest`, `DataLossRiskAssessorTest` pass.
--
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] Preserve serving replicas during best-efforts rebalance [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #18742:
URL: https://github.com/apache/pinot/pull/18742#issuecomment-5975815156
[@Jackie-Jiang](https://github.com/Jackie-Jiang), you are right that the previous change tightened the best-efforts contract. I narrowed the guard to the last observed serving replica: it allows 2→1 with a configured minimum of 2, safe partial progress, and moves for segments with no serving replicas. Serving sources are selected before assignment calculation, and unready/ERROR targets can load while those sources are retained. Strict replica groups retain a common serving source where one exists.
This still differs from `bestEfforts=false`, which stops on ERROR/nonconvergence. `FAILED` remains when no safe next assignment can advance: `DONE` would mark an incomplete target finished and suppress retries. `minAvailableReplicas=0` remains the explicit opt-out. Could you confirm whether this failure boundary is acceptable?
Updated in [9bded0fc1e](https://github.com/apache/pinot/commit/9bded0fc1eb4dfc3c332f2ae9313c25b2776c026), rebased onto current master. All 24 targeted tests, formatting/checkstyle/license checks, and independent review passed. The standard local reactor hits an unchanged `ZstandardDecompressor` missing-`NotNull` dependency; the passing run used a compiler-only JetBrains annotations classpath supplement. Fresh hosted CI is separate from these local results. Re-review requested.
--
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] Preserve serving replicas during best-efforts rebalance [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 closed pull request #18742: Preserve serving replicas during best-efforts rebalance
URL: https://github.com/apache/pinot/pull/18742
--
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 table rebalance to not drop the last serving replicas when proceeding on best-efforts without ExternalView convergence [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #18742:
URL: https://github.com/apache/pinot/pull/18742#issuecomment-4686257002
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/18742?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.95%. Comparing base ([`b0cb24b`](https://app.codecov.io/gh/apache/pinot/commit/b0cb24b0e89a352b0ad48ffa637fbf27504fc6bc?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`0d6a896`](https://app.codecov.io/gh/apache/pinot/commit/0d6a89690ec3f95c3c390e91431db272d7817c38?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 5 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (b0cb24b) and HEAD (0d6a896). Click for more details.
>
> <details><summary>HEAD has 16 uploads less than BASE</summary>
>
>| Flag | BASE (b0cb24b) | HEAD (0d6a896) |
>|------|------|------|
>|java-21|5|1|
>|unittests|2|1|
>|temurin|5|1|
>|unittests2|1|0|
>|integration|3|0|
>|integration1|1|0|
>|custom-integration1|1|0|
>|integration2|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #18742 +/- ##
============================================
- Coverage 64.59% 56.95% -7.65%
+ Complexity 1305 1 -1304
============================================
Files 3373 2591 -782
Lines 208894 151079 -57815
Branches 32667 24439 -8228
============================================
- Hits 134937 86048 -48889
+ Misses 63116 57739 -5377
+ Partials 10841 7292 -3549
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/18742/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/18742/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/18742/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/18742/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/18742/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/18742/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.95% <ø> (-7.65%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/18742/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.95% <ø> (-7.65%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/18742/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.95% <ø> (-7.64%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/18742/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.95% <ø> (-0.08%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/18742/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/18742?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 table rebalance to not drop the last serving replicas when proceeding on best-efforts without ExternalView convergence [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #18742:
URL: https://github.com/apache/pinot/pull/18742#issuecomment-5233949458
This is no longer `best-effort`, which shouldn't fail the rebalance in the middle.
I feel the described solution is not better than turning `best-effort` off
--
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