[kudu-CR] KUDU-2780: create thread for auto-rebalancing (1/n)
Hannah Nguyen has posted comments on this change. (
http://gerrit.cloudera.org:8080/13891 )
Change subject: KUDU-2780: create thread for auto-rebalancing (1/n)
..
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/13891/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13891/1//COMMIT_MSG@13
PS1, Line 13: Code that is modified/copied directly from the CLI rebalancing
: tool in kudu/tools is marked.
> Did you consider separating the rebalancing piece that doesn't depend on th
Hmmm, the point of this patch was mostly to lay the infrastructure (create the
Task in the master) by implementing some code that would log the current skew
of the cluster.
I don't think this patch will be merge-able, but the next one (which will
migrate any common code into the master directory, and retrieve information
from the master and not call 'ksck' at all) should be.
Will clarify this in the message.
http://gerrit.cloudera.org:8080/#/c/13891/1/src/kudu/master/CMakeLists.txt
File src/kudu/master/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/13891/1/src/kudu/master/CMakeLists.txt@61
PS1, Line 61: ksck
> This introduce a cycling dependency which is a no-no:
Will refactor the auto-rebalancer to not call 'ksck' at all.
Then moving common code into the master should also be ok, since tools already
depends on the master directory.
http://gerrit.cloudera.org:8080/#/c/13891/1/src/kudu/master/autorebalancer-test.cc
File src/kudu/master/autorebalancer-test.cc:
http://gerrit.cloudera.org:8080/#/c/13891/1/src/kudu/master/autorebalancer-test.cc@55
PS1, Line 55: TEST_F(AutoRebalancerTest, NoReports) {
> We have many tests which start master and does some work before shutting it
Agreed, this test isn't necessary.
http://gerrit.cloudera.org:8080/#/c/13891/1/src/kudu/master/autorebalancer-test.cc@69
PS1, Line 69: TEST_F(AutoRebalancerTest, ViewReports) {
> What does this test tries to verify?
This test keeps a cluster around for at least a couple polling intervals, and
since the auto-rebalancer is currently just logging the skew of the cluster,
running the test should print some cluster reports of skew.
I just looked at the output on my terminal manually. Is there a better way to
write the test to check that it appears?
http://gerrit.cloudera.org:8080/#/c/13891/1/src/kudu/master/autorebalancer.h
File src/kudu/master/autorebalancer.h:
http://gerrit.cloudera.org:8080/#/c/13891/1/src/kudu/master/autorebalancer.h@74
PS1, Line 74:
> nit: we use spaces, not tabs in Kudu C++ for indentation.
Done
--
To view, visit http://gerrit.cloudera.org:8080/13891
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc829d14dc8636287bf3a4faa4ba76e86ab6885c
Gerrit-Change-Number: 13891
Gerrit-PatchSet: 1
Gerrit-Owner: Hannah Nguyen
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Hannah Nguyen
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 23 Jul 2019 01:16:07 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2780: create thread for auto-rebalancing (1/n)
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/13891 )
Change subject: KUDU-2780: create thread for auto-rebalancing (1/n)
..
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/13891/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13891/1//COMMIT_MSG@13
PS1, Line 13: Code that is modified/copied directly from the CLI rebalancing
: tool in kudu/tools is marked.
Did you consider separating the rebalancing piece that doesn't depend on the
ksck structures per se into a library that might be used from both master and
the ksck CLI tool? Another option would be moving the whole rebalancing code
from the tools directory under master directory, and introducing an RPC
end-point in master that the CLI tool would simply call.
I'm not sure it's a good idea to keep duplicate code around.
http://gerrit.cloudera.org:8080/#/c/13891/1/src/kudu/master/CMakeLists.txt
File src/kudu/master/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/13891/1/src/kudu/master/CMakeLists.txt@61
PS1, Line 61: ksck
This introduce a cycling dependency which is a no-no:
12:59:16 CMake Error: The inter-target dependency graph contains the following
strongly connected component (cycle):
12:59:16 "master" of type SHARED_LIBRARY
12:59:16 depends on "ksck" (weak)
12:59:16 "ksck" of type SHARED_LIBRARY
12:59:16 depends on "master" (weak)
BTW, why does master need the ksck library as the link dependecney if the
rebalancer is being run as a simple out-of-the box binary?
http://gerrit.cloudera.org:8080/#/c/13891/1/src/kudu/master/autorebalancer-test.cc
File src/kudu/master/autorebalancer-test.cc:
http://gerrit.cloudera.org:8080/#/c/13891/1/src/kudu/master/autorebalancer-test.cc@55
PS1, Line 55: TEST_F(AutoRebalancerTest, NoReports) {
We have many tests which start master and does some work before shutting it
down. Given this scenario doesn't do much aside from that, I think we
automatically get coverage for this simple functionality already, no?
http://gerrit.cloudera.org:8080/#/c/13891/1/src/kudu/master/autorebalancer-test.cc@69
PS1, Line 69: TEST_F(AutoRebalancerTest, ViewReports) {
What does this test tries to verify?
http://gerrit.cloudera.org:8080/#/c/13891/1/src/kudu/master/autorebalancer.h
File src/kudu/master/autorebalancer.h:
http://gerrit.cloudera.org:8080/#/c/13891/1/src/kudu/master/autorebalancer.h@74
PS1, Line 74:
nit: we use spaces, not tabs in Kudu C++ for indentation.
--
To view, visit http://gerrit.cloudera.org:8080/13891
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc829d14dc8636287bf3a4faa4ba76e86ab6885c
Gerrit-Change-Number: 13891
Gerrit-PatchSet: 1
Gerrit-Owner: Hannah Nguyen
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 23 Jul 2019 00:56:44 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2780: create thread for auto-rebalancing (1/n)
Hannah Nguyen has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13891 Change subject: KUDU-2780: create thread for auto-rebalancing (1/n) .. KUDU-2780: create thread for auto-rebalancing (1/n) AutoRebalancerTask is a thread in the master with a configurable polling interval. Every interval, the thread logs the current skew of the cluster if auto-rebalancing is enabled. Code that is modified/copied directly from the CLI rebalancing tool in kudu/tools is marked. The task currently calls the 'ksck' tool to determine skew, but this may change in later patches when rebalancing is implemented and the task retrieves information directly from the master. Change-Id: Ibc829d14dc8636287bf3a4faa4ba76e86ab6885c --- M src/kudu/benchmarks/tpch/tpch1.cc M src/kudu/master/CMakeLists.txt A src/kudu/master/autorebalancer-test.cc A src/kudu/master/autorebalancer.cc A src/kudu/master/autorebalancer.h M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h 7 files changed, 1,769 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/13891/1 -- To view, visit http://gerrit.cloudera.org:8080/13891 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ibc829d14dc8636287bf3a4faa4ba76e86ab6885c Gerrit-Change-Number: 13891 Gerrit-PatchSet: 1 Gerrit-Owner: Hannah Nguyen
