You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2018/11/02 07:57:41 UTC
[kudu-CR] [rebalancer] location-aware rebalancer (part 9/n)
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11862
Change subject: [rebalancer] location-aware rebalancer (part 9/n)
......................................................................
[rebalancer] location-aware rebalancer (part 9/n)
Updated reporting functionality of the rebalancer tool to output
information on placement policy violations and other relevant
information for location-aware clusters.
Added one simple integration test as well.
Change-Id: I8407e9f8cf6b41a6aeb075372d852125d9739e08
---
M src/kudu/tools/placement_policy_util.cc
M src/kudu/tools/rebalancer.cc
M src/kudu/tools/rebalancer.h
M src/kudu/tools/rebalancer_tool-test.cc
4 files changed, 432 insertions(+), 104 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/11862/1
--
To view, visit http://gerrit.cloudera.org:8080/11862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8407e9f8cf6b41a6aeb075372d852125d9739e08
Gerrit-Change-Number: 11862
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [rebalancer] location-aware rebalancer (part 9/n)
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11862 )
Change subject: [rebalancer] location-aware rebalancer (part 9/n)
......................................................................
[rebalancer] location-aware rebalancer (part 9/n)
Updated reporting functionality of the rebalancer tool to output
information on placement policy violations and other relevant
information for location-aware clusters.
Added one simple integration test as well.
Change-Id: I8407e9f8cf6b41a6aeb075372d852125d9739e08
Reviewed-on: http://gerrit.cloudera.org:8080/11862
Tested-by: Kudu Jenkins
Reviewed-by: Will Berkeley <wd...@gmail.com>
---
M src/kudu/tools/placement_policy_util-test.cc
M src/kudu/tools/placement_policy_util.cc
M src/kudu/tools/placement_policy_util.h
M src/kudu/tools/rebalancer.cc
M src/kudu/tools/rebalancer.h
M src/kudu/tools/rebalancer_tool-test.cc
6 files changed, 457 insertions(+), 141 deletions(-)
Approvals:
Kudu Jenkins: Verified
Will Berkeley: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/11862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I8407e9f8cf6b41a6aeb075372d852125d9739e08
Gerrit-Change-Number: 11862
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [rebalancer] location-aware rebalancer (part 9/n)
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11862
to look at the new patch set (#2).
Change subject: [rebalancer] location-aware rebalancer (part 9/n)
......................................................................
[rebalancer] location-aware rebalancer (part 9/n)
Updated reporting functionality of the rebalancer tool to output
information on placement policy violations and other relevant
information for location-aware clusters.
Added one simple integration test as well.
Change-Id: I8407e9f8cf6b41a6aeb075372d852125d9739e08
---
M src/kudu/tools/placement_policy_util-test.cc
M src/kudu/tools/placement_policy_util.cc
M src/kudu/tools/placement_policy_util.h
M src/kudu/tools/rebalancer.cc
M src/kudu/tools/rebalancer.h
M src/kudu/tools/rebalancer_tool-test.cc
6 files changed, 455 insertions(+), 139 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/11862/2
--
To view, visit http://gerrit.cloudera.org:8080/11862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8407e9f8cf6b41a6aeb075372d852125d9739e08
Gerrit-Change-Number: 11862
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [rebalancer] location-aware rebalancer (part 9/n)
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11862 )
Change subject: [rebalancer] location-aware rebalancer (part 9/n)
......................................................................
Patch Set 2:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/11862/2/src/kudu/tools/placement_policy_util-test.cc
File src/kudu/tools/placement_policy_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/11862/2/src/kudu/tools/placement_policy_util-test.cc@142
PS2, Line 142: default comparision
: // operator
There's a default comparison operator in C++? I didn't think there was. How it should it work? I don't think we should use it, even in a test.
I think tuples have a comparison operator if their components do, though.
http://gerrit.cloudera.org:8080/#/c/11862/2/src/kudu/tools/placement_policy_util-test.cc@143
PS2, Line 143: nonsence
nonsense
http://gerrit.cloudera.org:8080/#/c/11862/2/src/kudu/tools/rebalancer.cc
File src/kudu/tools/rebalancer.cc:
http://gerrit.cloudera.org:8080/#/c/11862/2/src/kudu/tools/rebalancer.cc@129
PS2, Line 129: iformation
information
http://gerrit.cloudera.org:8080/#/c/11862/2/src/kudu/tools/rebalancer.cc@137
PS2, Line 137: vilations
violations
http://gerrit.cloudera.org:8080/#/c/11862/2/src/kudu/tools/rebalancer.cc@138
PS2, Line 138: RETURN_NOT_OK(PrintPolicyViolationInfo(raw_info, out));
It looks like the output could be kind of long. Can we move placement policy violations to the bottom, where they're most likely to be seen when this is run from the command line. Alternatively, you can leave them at the top, but print an extra message at the bottom if there are placement policy violations, one directing users to look up here for details.
http://gerrit.cloudera.org:8080/#/c/11862/2/src/kudu/tools/rebalancer.cc@145
PS2, Line 145: sorated
sorted
--
To view, visit http://gerrit.cloudera.org:8080/11862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8407e9f8cf6b41a6aeb075372d852125d9739e08
Gerrit-Change-Number: 11862
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 02 Nov 2018 18:41:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] [rebalancer] location-aware rebalancer (part 9/n)
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11862 )
Change subject: [rebalancer] location-aware rebalancer (part 9/n)
......................................................................
Patch Set 3:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/11862/2/src/kudu/tools/placement_policy_util-test.cc
File src/kudu/tools/placement_policy_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/11862/2/src/kudu/tools/placement_policy_util-test.cc@142
PS2, Line 142: lhs,
:
> There's a default comparison operator in C++? I didn't think there was. How
Woops, it seems I confused that with default copy constructor which works on field-by-field basis. It seems I was too grumpy yesterday night :)
http://gerrit.cloudera.org:8080/#/c/11862/2/src/kudu/tools/placement_policy_util-test.cc@143
PS2, Line 143: hs) {
> nonsense
Done
http://gerrit.cloudera.org:8080/#/c/11862/2/src/kudu/tools/rebalancer.cc
File src/kudu/tools/rebalancer.cc:
http://gerrit.cloudera.org:8080/#/c/11862/2/src/kudu/tools/rebalancer.cc@129
PS2, Line 129: information
> information
Done
http://gerrit.cloudera.org:8080/#/c/11862/2/src/kudu/tools/rebalancer.cc@137
PS2, Line 137: lance.
> violations
Done
http://gerrit.cloudera.org:8080/#/c/11862/2/src/kudu/tools/rebalancer.cc@138
PS2, Line 138: RETURN_NOT_OK(PrintCrossLocationBalanceStats(ci, out));
> It looks like the output could be kind of long. Can we move placement polic
Done
http://gerrit.cloudera.org:8080/#/c/11862/2/src/kudu/tools/rebalancer.cc@145
PS2, Line 145: ons.res
> sorted
Done
--
To view, visit http://gerrit.cloudera.org:8080/11862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8407e9f8cf6b41a6aeb075372d852125d9739e08
Gerrit-Change-Number: 11862
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 02 Nov 2018 23:30:20 +0000
Gerrit-HasComments: Yes
[kudu-CR] [rebalancer] location-aware rebalancer (part 9/n)
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11862 )
Change subject: [rebalancer] location-aware rebalancer (part 9/n)
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/11862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8407e9f8cf6b41a6aeb075372d852125d9739e08
Gerrit-Change-Number: 11862
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 06 Nov 2018 21:54:27 +0000
Gerrit-HasComments: No
[kudu-CR] [rebalancer] location-aware rebalancer (part 9/n)
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11862
to look at the new patch set (#3).
Change subject: [rebalancer] location-aware rebalancer (part 9/n)
......................................................................
[rebalancer] location-aware rebalancer (part 9/n)
Updated reporting functionality of the rebalancer tool to output
information on placement policy violations and other relevant
information for location-aware clusters.
Added one simple integration test as well.
Change-Id: I8407e9f8cf6b41a6aeb075372d852125d9739e08
---
M src/kudu/tools/placement_policy_util-test.cc
M src/kudu/tools/placement_policy_util.cc
M src/kudu/tools/placement_policy_util.h
M src/kudu/tools/rebalancer.cc
M src/kudu/tools/rebalancer.h
M src/kudu/tools/rebalancer_tool-test.cc
6 files changed, 457 insertions(+), 141 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/11862/3
--
To view, visit http://gerrit.cloudera.org:8080/11862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8407e9f8cf6b41a6aeb075372d852125d9739e08
Gerrit-Change-Number: 11862
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>