Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/23909 )
Change subject: KUDU-3728 Add range-aware rebalancing to auto_rebalancer ...................................................................... Patch Set 5: (10 comments) http://gerrit.cloudera.org:8080/#/c/23909/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/23909/5//COMMIT_MSG@16 PS5, Line 16: tabletsa : re unexpected line break? http://gerrit.cloudera.org:8080/#/c/23909/5/src/kudu/master/auto_rebalancer-test.cc File src/kudu/master/auto_rebalancer-test.cc: http://gerrit.cloudera.org:8080/#/c/23909/5/src/kudu/master/auto_rebalancer-test.cc@267 PS5, Line 267: std::map nit: add 'using std::map;' in the beginning of this file and drop the namespace prefix http://gerrit.cloudera.org:8080/#/c/23909/5/src/kudu/master/auto_rebalancer-test.cc@270 PS5, Line 270: std:: nit: there is 'using std::vector', so no namespace prefix is necessary http://gerrit.cloudera.org:8080/#/c/23909/5/src/kudu/master/auto_rebalancer-test.cc@276 PS5, Line 276: std:: ditto for the namespace prefix http://gerrit.cloudera.org:8080/#/c/23909/5/src/kudu/master/auto_rebalancer-test.cc@287 PS5, Line 287: std::map ditto for the namespace prefix http://gerrit.cloudera.org:8080/#/c/23909/5/src/kudu/master/auto_rebalancer-test.cc@288 PS5, Line 288: auto& elem nit: consider using structured binding here for (const auto& [tag, counts_by_ts] : counts_by_tag) { ... } http://gerrit.cloudera.org:8080/#/c/23909/5/src/kudu/master/auto_rebalancer-test.cc@368 PS5, Line 368: if (auto_rebalancing_enable_range_prev_) { : FLAGS_auto_rebalancing_enable_range_rebalancing = : *auto_rebalancing_enable_range_prev_; : } : if (auto_rebalancing_enabled_prev_) { : FLAGS_auto_rebalancing_enabled = *auto_rebalancing_enabled_prev_; : } : if (enable_range_replica_placement_prev_) { : FLAGS_enable_range_replica_placement = : *enable_range_replica_placement_prev_; : } : if (enable_minidumps_prev_) { : FLAGS_enable_minidumps = *enable_minidumps_prev_; : } I didn't pay attention to this in the prior review rounds, but I just realized that maybe this isn't necessary if you use google::FlagSaver where necessary. Did you try using it instead of this explicit code? For reference, you could check out how it's used in other tests in the Kudu's codebase. http://gerrit.cloudera.org:8080/#/c/23909/5/src/kudu/master/auto_rebalancer-test.cc@658 PS5, Line 658: TEST_F(AutoRebalancerTest, RangeAwareRebalancesNewRangeReplicas) { Thank you for adding this new scenario! http://gerrit.cloudera.org:8080/#/c/23909/5/src/kudu/master/auto_rebalancer-test.cc@709 PS5, Line 709: std:: nit: you could omit this because there is corresponding 'using' directive in the beginning of this file http://gerrit.cloudera.org:8080/#/c/23909/1/src/kudu/master/auto_rebalancer.cc File src/kudu/master/auto_rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/23909/1/src/kudu/master/auto_rebalancer.cc@614 PS1, Line 614: nge_key_begin; > The two casts serve different purposes: the uint8_t ensures the byte is tre Yes, I see. But if you want this to be consistent with the output from ksck (at least that's what the comment at line 614 of PS1 says), then it's necessary to have just a single static_cast<uint16_t> and allow for signed extension. If you want to change this behavior and keep the representation of range key's elements in the 0-255 interval when printing their values in hex, I'd suggest to address this here and in ksck in a separate changelist. That way it would be more consistent IMO. What do you think? -- To view, visit http://gerrit.cloudera.org:8080/23909 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idf716bef8c21395da98dd7c9e33001190d9b5587 Gerrit-Change-Number: 23909 Gerrit-PatchSet: 5 Gerrit-Owner: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 03 Mar 2026 06:36:24 +0000 Gerrit-HasComments: Yes
