Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14110 )
Change subject: Create kudu/rebalance subdirectory ...................................................................... Patch Set 9: (8 comments) http://gerrit.cloudera.org:8080/#/c/14110/9/src/kudu/rebalance/cluster_status.h File src/kudu/rebalance/cluster_status.h: http://gerrit.cloudera.org:8080/#/c/14110/9/src/kudu/rebalance/cluster_status.h@36 PS9, Line 36: // The result of health check on a tablet. : // Also used to indicate the health of a table, since the health of a table is : // the health of its least healthy tablet. : enum class ClusterCheckResult { : // The tablet is healthy. : HEALTHY, : : // The tablet has on-going tablet copies. : RECOVERING, : : // The tablet has fewer replicas than its table's replication factor and : // has no on-going tablet copies. : UNDER_REPLICATED, : : // The tablet is missing a majority of its replicas and is unavailable for : // writes. If a majority cannot be brought back online, then the tablet : // requires manual intervention to recover. : UNAVAILABLE, : : // There was a discrepancy among the tablets' consensus configs and the master's. : CONSENSUS_MISMATCH, : }; I'm not sure this fits exactly under 'rebalance' namespace. The same concern for other former ksck stuff in this file. Maybe, create a separate namespace 'cluster' and put that stuff there? There are might be other options around. In the worst case, I'm not against keeping it here for a while, but the placing it under 'rebalance' namespace is a bit odd since this is used by the ksck CLI tool in the context far from rebalancing. Once put under 'cluster' namespace or alike, it would be natural to drop 'Cluster' prefix in the names of corresponding structures. http://gerrit.cloudera.org:8080/#/c/14110/9/src/kudu/rebalance/rebalance-test.cc File src/kudu/rebalance/rebalance-test.cc: http://gerrit.cloudera.org:8080/#/c/14110/9/src/kudu/rebalance/rebalance-test.cc@182 PS9, Line 182: return HasSameContents(lhs.servers_by_replica_count, : rhs.servers_by_replica_count); nit: indentation http://gerrit.cloudera.org:8080/#/c/14110/9/src/kudu/rebalance/rebalancer.h File src/kudu/rebalance/rebalancer.h: http://gerrit.cloudera.org:8080/#/c/14110/9/src/kudu/rebalance/rebalancer.h@19 PS9, Line 19: #include <stddef.h> : #include <stdint.h> nit here and elsewhere when using IWYU suggestions: please use C++-style include headers instead, like #include <cstddef> #include <cstdint> IWYU recommends the universal C-style ones, and we haven't fixed that in IWYU yet. Once you switch to C++-style ones, re-order the includes to make sure the sorted alphabetically. http://gerrit.cloudera.org:8080/#/c/14110/9/src/kudu/rebalance/rebalancer.h@47 PS9, Line 47: class Rebalancer { Add short documentation for the class. The important point is to explain what are responsibilities and functionality of this class. Also, what is behind separation of this class out of the RebalancerTool ? Does it serve any other logical functionality in addition to pure namespacing? If not, maybe you could simply extract the classes/structs and those static methods directly under kudu::rebalance namespace. http://gerrit.cloudera.org:8080/#/c/14110/9/src/kudu/rebalance/rebalancer.cc File src/kudu/rebalance/rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/14110/9/src/kudu/rebalance/rebalancer.cc@57 PS9, Line 57: std::vector<std::string> ignored_tservers_param, : std::vector<std::string> master_addresses, : std::vector<std::string> table_filters, nit: remove std:: namespace prefix since there are corresponding 'using std::xxx' directives above. http://gerrit.cloudera.org:8080/#/c/14110/9/src/kudu/tools/ksck_results.cc File src/kudu/tools/ksck_results.cc: http://gerrit.cloudera.org:8080/#/c/14110/9/src/kudu/tools/ksck_results.cc@714 PS9, Line 714: nit: indent/spacing http://gerrit.cloudera.org:8080/#/c/14110/9/src/kudu/tools/ksck_results.cc@746 PS9, Line 746: nit: indent/spacing http://gerrit.cloudera.org:8080/#/c/14110/9/src/kudu/tools/ksck_results.cc@781 PS9, Line 781: nit: indent/spacing -- 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: 9 Gerrit-Owner: Hannah Nguyen <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hannah Nguyen <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 23 Aug 2019 19:15:26 +0000 Gerrit-HasComments: Yes
