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