Copilot commented on code in PR #19069:
URL: https://github.com/apache/pinot/pull/19069#discussion_r3669208990
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -812,27 +816,105 @@ private RebalanceResult doRebalance(TableConfig
tableConfig, RebalanceConfig reb
idealState.setNumPartitions(nextAssignment.size());
idealState.setReplicas(Integer.toString(nextAssignment.values().iterator().next().size()));
- // Check version and update IdealState
- try {
- Preconditions.checkState(_helixDataAccessor.getBaseDataAccessor()
- .set(idealStatePropertyKey.getPath(), idealStateRecord,
expectedVersion, AccessOption.PERSISTENT),
- "Failed to update IdealState");
+ // Segments this batch changes relative to the current assignment it was
computed against. Captured before the
+ // compare-and-set so that, on a version conflict, we can tell whether a
concurrent write touched any segment
+ // this batch moves.
+ List<String> batchMovedSegments =
SegmentAssignmentUtils.getSegmentsToMove(currentAssignment, nextAssignment);
+
+ // Check version and update the IdealState. If the compare-and-set fails
only because a concurrent write bumped
+ // the version without touching the segments this batch moves (e.g.
consuming segment commits on a continuously
+ // ingesting table), rebase this batch onto the latest IdealState and
retry the compare-and-set in place, without
+ // waiting for the ExternalView to converge again (this batch never
landed, so there is nothing new to wait for)
+ // or recomputing the full target assignment. This keeps the rebalance
from live-locking against a steady stream
+ // of version bumps. Only attempted when this rebalance moves only tier
segments: the base placements are then
+ // unchanged, so a segment added concurrently keeps its correct
placement and can be carried over as-is.
+ boolean rebasable = isMovingOnlyTierSegments(segmentsToMove,
providedTierToSegmentsMap);
+ boolean updated = false;
+ int rebaseAttempts = 0;
+ while (true) {
Review Comment:
`expectedVersion` is later mutated during rebase attempts (set to the latest
IdealState version) even if the update ultimately fails. That breaks the
invariant that `expectedVersion` matches the version of `currentAssignment`
(which is only updated on a successful write or when the version-mismatch block
runs). If rebasing hits the max attempts and falls back, the next loop
iteration can incorrectly skip the version-mismatch path and continue with a
stale `currentAssignment`/`targetAssignment`. Capture the original version
before attempting rebases so it can be restored on failure.
This issue also appears on line 914 of the same file.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -812,27 +816,105 @@ private RebalanceResult doRebalance(TableConfig
tableConfig, RebalanceConfig reb
idealState.setNumPartitions(nextAssignment.size());
idealState.setReplicas(Integer.toString(nextAssignment.values().iterator().next().size()));
- // Check version and update IdealState
- try {
- Preconditions.checkState(_helixDataAccessor.getBaseDataAccessor()
- .set(idealStatePropertyKey.getPath(), idealStateRecord,
expectedVersion, AccessOption.PERSISTENT),
- "Failed to update IdealState");
+ // Segments this batch changes relative to the current assignment it was
computed against. Captured before the
+ // compare-and-set so that, on a version conflict, we can tell whether a
concurrent write touched any segment
+ // this batch moves.
+ List<String> batchMovedSegments =
SegmentAssignmentUtils.getSegmentsToMove(currentAssignment, nextAssignment);
+
+ // Check version and update the IdealState. If the compare-and-set fails
only because a concurrent write bumped
+ // the version without touching the segments this batch moves (e.g.
consuming segment commits on a continuously
+ // ingesting table), rebase this batch onto the latest IdealState and
retry the compare-and-set in place, without
+ // waiting for the ExternalView to converge again (this batch never
landed, so there is nothing new to wait for)
+ // or recomputing the full target assignment. This keeps the rebalance
from live-locking against a steady stream
+ // of version bumps. Only attempted when this rebalance moves only tier
segments: the base placements are then
+ // unchanged, so a segment added concurrently keeps its correct
placement and can be carried over as-is.
Review Comment:
This change adds fairly subtle state-machine behavior (rebasing an
IdealState batch update on `ZkBadVersionException`, plus refreshing
`targetAssignment` after adopting a newer IdealState). There isn’t a unit test
covering the version-conflict/rebase path today, even though `pinot-controller`
already has `TableRebalancerTest`. Adding a regression test that forces a
version bump between the initial read and the compare-and-set (and asserts the
rebased update preserves disjoint concurrent changes and keeps observer stats
consistent) would help prevent future regressions.
--
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]
