Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24525 )
Change subject: KUDU-3789 auto_rebalancer: reliably clear replace marker after failed moves ...................................................................... Patch Set 3: Code-Review+1 (2 comments) http://gerrit.cloudera.org:8080/#/c/24525/2/src/kudu/master/auto_rebalancer.cc File src/kudu/master/auto_rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/24525/2/src/kudu/master/auto_rebalancer.cc@1079 PS2, Line 1079: HostPort leader_hp; : // Resolving the leader also re-checks our own catalog leadership (it takes the : // leader lock), so if we've lost it here, or there's no tablet leader yet, we : // bail out and the caller leaves the marker pending. : RETURN_NOT_OK(GetTabletLeader(move.tablet_uuid, &leader_uuid, &leader_hp)); : vector<Sockaddr> resolved; : RETURN_NOT_OK(leader_hp.ResolveAddre > Agreed. I've added a CAS on the clear, so it only removes the marker if the Thank you for the update. It should help a bit, yes. If I'm not mistaken, even with this extra check there is still a TOCTOU-style race, especially when TryClearReplaceMarker() is called later on from ProcessPendingReplaceClears() which is invoked from AutoRebalancerTask::RunLoop() on its turn. Consider adding a comment about this -- it would be a bit more clear for a reader. I don't have an ultimate solution for this based on how ChangeConfigResponsePB requests are processed. And we cannot use the CAS info from ReplicaMove even if it's present, right? I think this should not be a problem -- after refreshing my knowledge how tablet replicas are replaced, I realized that a mistaken/spurious removal of the 'REPLACE' tag shouldn't leave the affected tablet under-replicated forever. http://gerrit.cloudera.org:8080/#/c/24525/2/src/kudu/master/auto_rebalancer.cc@1102 PS2, Line 1102: req.add_tablet_ids(move.tablet_uuid); : RETURN_NOT_OK(proxy.GetConsensusState(req, &resp, &rpc)); : if (resp.has_error()) { : return StatusFromPB(resp.error().status()); > Good question. The clear path re-checks leadership itself: TryClearReplaceM This makes sense, thanks for the clarification. -- To view, visit http://gerrit.cloudera.org:8080/24525 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia47698207612252bca3ec70fc090a37bd3f87809 Gerrit-Change-Number: 24525 Gerrit-PatchSet: 3 Gerrit-Owner: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Wed, 15 Jul 2026 22:42:36 +0000 Gerrit-HasComments: Yes
