You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Yifan Zhang (Code Review)" <ge...@cloudera.org> on 2019/10/21 13:06:21 UTC
[kudu-CR] rebalancer tool: limit max moves per server for PolicyFixer
Yifan Zhang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14519
Change subject: rebalancer_tool: limit max_moves_per_server for PolicyFixer
......................................................................
rebalancer_tool: limit max_moves_per_server for PolicyFixer
This patch completed the TODO in PolicyFixer and did some
other cleanup in rebalancer_tool.
Change-Id: I3499299d1fb6bebae7351a1c7a7c9b114990db89
---
M src/kudu/tools/rebalancer_tool.cc
M src/kudu/tools/rebalancer_tool.h
2 files changed, 45 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/14519/1
--
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: newchange
Gerrit-Change-Id: I3499299d1fb6bebae7351a1c7a7c9b114990db89
Gerrit-Change-Number: 14519
Gerrit-PatchSet: 1
Gerrit-Owner: Yifan Zhang <ch...@163.com>
[kudu-CR] rebalancer tool: limit max moves per server for PolicyFixer
Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Yifan Zhang 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 3:
(5 comments)
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: Rebalancer::ReplicaMove move);
> Modernize the signature to use the move semantics?
Done
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: move_info.tablet_uuid, move_info.ts_uuid_from);
: UpdateOnMoveScheduled(std::move(move_info));
> If using move semantics, log about the move before calling UpdateOnMoveSche
Done
http://gerrit.cloudera.org:8080/#/c/14519/2/src/kudu/tools/rebalancer_tool.cc@1285
PS2, Line 1285: max_moves_per_server_ /
> nit: add spaces
Done
http://gerrit.cloudera.org:8080/#/c/14519/2/src/kudu/tools/rebalancer_tool.cc@1295
PS2, Line 1295: const auto tablet_uuid = move.tablet_uuid;
> nit: add 'const' to be explicit it doesn't change in this scope?
Done
http://gerrit.cloudera.org:8080/#/c/14519/2/src/kudu/tools/rebalancer_tool.cc@1296
PS2, Line 1296: cons
> ditto
Done
--
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: 3
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Wed, 23 Oct 2019 03:51:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] rebalancer tool: limit max moves per server for PolicyFixer
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14519 )
Change subject: rebalancer_tool: limit max_moves_per_server for PolicyFixer
......................................................................
rebalancer_tool: limit max_moves_per_server for PolicyFixer
This patch completed the TODO in PolicyFixer and did some
other cleanup in rebalancer_tool.
Change-Id: I3499299d1fb6bebae7351a1c7a7c9b114990db89
Reviewed-on: http://gerrit.cloudera.org:8080/14519
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/tools/rebalancer_tool.cc
M src/kudu/tools/rebalancer_tool.h
2 files changed, 45 insertions(+), 27 deletions(-)
Approvals:
Kudu Jenkins: Verified
Alexey Serbin: Looks good to me, approved
--
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: merged
Gerrit-Change-Id: I3499299d1fb6bebae7351a1c7a7c9b114990db89
Gerrit-Change-Number: 14519
Gerrit-PatchSet: 4
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
[kudu-CR] rebalancer tool: limit max moves per server for PolicyFixer
Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14519
to look at the new patch set (#3).
Change subject: rebalancer_tool: limit max_moves_per_server for PolicyFixer
......................................................................
rebalancer_tool: limit max_moves_per_server for PolicyFixer
This patch completed the TODO in PolicyFixer and did some
other cleanup in rebalancer_tool.
Change-Id: I3499299d1fb6bebae7351a1c7a7c9b114990db89
---
M src/kudu/tools/rebalancer_tool.cc
M src/kudu/tools/rebalancer_tool.h
2 files changed, 45 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/14519/3
--
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: newpatchset
Gerrit-Change-Id: I3499299d1fb6bebae7351a1c7a7c9b114990db89
Gerrit-Change-Number: 14519
Gerrit-PatchSet: 3
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] rebalancer tool: limit max moves per server for PolicyFixer
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 3: Code-Review+2
--
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: 3
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Wed, 23 Oct 2019 22:07:18 +0000
Gerrit-HasComments: No
[kudu-CR] rebalancer tool: limit max moves per server for PolicyFixer
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <ch...@163.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 22 Oct 2019 17:58:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] rebalancer tool: limit max moves per server for PolicyFixer
Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14519
to look at the new patch set (#2).
Change subject: rebalancer_tool: limit max_moves_per_server for PolicyFixer
......................................................................
rebalancer_tool: limit max_moves_per_server for PolicyFixer
This patch completed the TODO in PolicyFixer and did some
other cleanup in rebalancer_tool.
Change-Id: I3499299d1fb6bebae7351a1c7a7c9b114990db89
---
M src/kudu/tools/rebalancer_tool.cc
M src/kudu/tools/rebalancer_tool.h
2 files changed, 45 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/14519/2
--
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: newpatchset
Gerrit-Change-Id: I3499299d1fb6bebae7351a1c7a7c9b114990db89
Gerrit-Change-Number: 14519
Gerrit-PatchSet: 2
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)