Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24285 )
Change subject: KUDU-3732 Get consensus state from CatalogManager in auto-rebalancer ...................................................................... Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/24285/3/src/kudu/master/auto_rebalancer.cc File src/kudu/master/auto_rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/24285/3/src/kudu/master/auto_rebalancer.cc@557 PS3, Line 557: // Record the config opid_index that was current before this move was : // dispatched. CheckMoveCompleted uses this to determine whether : // CatalogManager has received the heartbeat reflecting the new config. : move_info.config_opid_idx = pre_opid_index; With the introduction of this new logic, there is now a possibility of a TOCTOU-style race. There might be other actors that modify tablet's Raft config concurrently with this code, e.g. CLI-based rebalancer tool running and/or the auto-healing activity performed by the system catalog itself when it spots a non-responsive replica on inaccessible tablet server. Essentially, there might be a few updates in flight right after capturing this index, and the actual updated index wouldn't be 'pre_opid_index + 1' but rather 'pre_opid_index + 2' or even 'pre_opid_index + 3' in the new logic implemented in CheckMoveCompleted(). I think it's necessary either start populating the BulkChangeConfigRequestPB.cas_config_opid_index field and handle CAS-related failures here, or make the logic in CheckMoveCompleted() resistant to situations of concurrent updates where 'pre_opid_index + 1' is still a stale config index not reflecting the update made. I guess the former is the preferred way of addressing this issue. What do you think? http://gerrit.cloudera.org:8080/#/c/24285/3/src/kudu/master/auto_rebalancer.cc@921 PS3, Line 921: replica_move.config_opid_idx Could it happen that 'config_opid_idx' field is absent in the 'replica_move' structure? If yes, does the updated code work as expected in that case? If not, shouldn't there be DCHECK() of some other sort of debug-style assertion here? -- To view, visit http://gerrit.cloudera.org:8080/24285 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba021539fbfaca5905fbbf4abbb6e2a61db1f7dc Gerrit-Change-Number: 24285 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-Comment-Date: Tue, 26 May 2026 20:47:56 +0000 Gerrit-HasComments: Yes
