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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
