Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14519 )
Change subject: rebalancer_tool: limit max_moves_per_server for PolicyFixer ...................................................................... Patch Set 2: (6 comments) Thank you for addressing these. Overall looks good, just a few nits. http://gerrit.cloudera.org:8080/#/c/14519/2/src/kudu/tools/rebalancer_tool.h File src/kudu/tools/rebalancer_tool.h: http://gerrit.cloudera.org:8080/#/c/14519/2/src/kudu/tools/rebalancer_tool.h@292 PS2, Line 292: const Rebalancer::ReplicaMove& move Modernize the signature to use the move semantics? I.e. 'void UpdateOnMoveScheduled(Rebalancer::ReplicaMove)', and in call sites use 'UpdateOnMoveScheduled(std::move(...))' http://gerrit.cloudera.org:8080/#/c/14519/2/src/kudu/tools/rebalancer_tool.cc File src/kudu/tools/rebalancer_tool.cc: http://gerrit.cloudera.org:8080/#/c/14519/2/src/kudu/tools/rebalancer_tool.cc@1164 PS2, Line 1164: LOG(INFO) << Substitute("tablet $0: '$1' -> '?' move scheduled", : move_info.tablet_uuid, move_info.ts_uuid_from); If using move semantics, log about the move before calling UpdateOnMoveSchedule() http://gerrit.cloudera.org:8080/#/c/14519/2/src/kudu/tools/rebalancer_tool.cc@1285 PS2, Line 1285: max_moves_per_server_/2 nit: add spaces max_moves_per_server_ / 2 http://gerrit.cloudera.org:8080/#/c/14519/2/src/kudu/tools/rebalancer_tool.cc@1295 PS2, Line 1295: auto tablet_uuid = move.tablet_uuid; nit: add 'const' to be explicit it doesn't change in this scope? http://gerrit.cloudera.org:8080/#/c/14519/2/src/kudu/tools/rebalancer_tool.cc@1296 PS2, Line 1296: auto ditto http://gerrit.cloudera.org:8080/#/c/14519/2/src/kudu/tools/rebalancer_tool.cc@1314 PS2, Line 1314: // Update helper containers. Good catch! -- To view, visit http://gerrit.cloudera.org:8080/14519 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3499299d1fb6bebae7351a1c7a7c9b114990db89 Gerrit-Change-Number: 14519 Gerrit-PatchSet: 2 Gerrit-Owner: Yifan Zhang <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 22 Oct 2019 17:58:12 +0000 Gerrit-HasComments: Yes
