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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Hannah Nguyen <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 23 Jul 2019 01:16:07 +0000 Gerrit-HasComments: Yes
