Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14110 )
Change subject: Move some rebalancing logic from tools to master ...................................................................... Patch Set 2: (4 comments) I took a quick look and I think it's a good first step in the right direction. It would be great to move the rest of the rebalancer- and policy-placement related stuff as well. http://gerrit.cloudera.org:8080/#/c/14110/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14110/2//COMMIT_MSG@15 PS2, Line 15: TODO: move tools/placement_policy_util and tools/rebalancer : into master too, so future auto-rebalancer task does not : rely on tools. Yep, it makes sense to do so. Probably, it's best to do so in this changelist, otherwise the changelist doesn't seem to be logically complete. http://gerrit.cloudera.org:8080/#/c/14110/2/src/kudu/master/CMakeLists.txt File src/kudu/master/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/14110/2/src/kudu/master/CMakeLists.txt@48 PS2, Line 48: rebalance_algo.cc You might separate the rebalancing and replica placement stuff (i.e. placement_policy.cc) into a separate library. Link it into the tools instead of linking in libmaster. That might with resolving circular dependencies (if any) while trying to move the rest of rebalancer stuff from tools into the master. http://gerrit.cloudera.org:8080/#/c/14110/2/src/kudu/master/cluster_status.h File src/kudu/master/cluster_status.h: http://gerrit.cloudera.org:8080/#/c/14110/2/src/kudu/master/cluster_status.h@33 PS2, Line 33: namespace master { nit: I'm not sure that 'master' is the best namespace to host cluster-related stuff. Maybe, leave it just under 'kudu' namespace? What do you think? http://gerrit.cloudera.org:8080/#/c/14110/2/src/kudu/master/cluster_status.cc File src/kudu/master/cluster_status.cc: http://gerrit.cloudera.org:8080/#/c/14110/2/src/kudu/master/cluster_status.cc@72 PS2, Line 72: int ServerHealthScore(ClusterServerHealth sh) { : switch (sh) { : case ClusterServerHealth::HEALTHY: : return 0; : case ClusterServerHealth::UNAUTHORIZED: : return 1; : case ClusterServerHealth::UNAVAILABLE: : return 2; : case ClusterServerHealth::WRONG_SERVER_UUID: : return 3; : default: : LOG(FATAL) << "Unknown ClusterServerHealth"; I know this just came from the prior revision, but I'm not sure this is needed. The ClusterServerHealth is backed by int anyways, so I would expect its elements are comparable without any additional tricks. I think you can remove this safely. -- To view, visit http://gerrit.cloudera.org:8080/14110 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie0242a019cb44517539da2878cf889ee0c511964 Gerrit-Change-Number: 14110 Gerrit-PatchSet: 2 Gerrit-Owner: Hannah Nguyen <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Hannah Nguyen <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 21 Aug 2019 17:50:14 +0000 Gerrit-HasComments: Yes
