Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14177 )
Change subject: KUDU-2780: create thread for auto-rebalancing ...................................................................... Patch Set 25: (6 comments) http://gerrit.cloudera.org:8080/#/c/14177/23/src/kudu/master/auto_rebalancer-test.cc File src/kudu/master/auto_rebalancer-test.cc: http://gerrit.cloudera.org:8080/#/c/14177/23/src/kudu/master/auto_rebalancer-test.cc@524 PS23, Line 524: "could not perform replica move: Network error"); > Ah, thanks for the clarification. Indeed, the idea was as Andrew described. Thank you for addressing this -- not it makes more sense to me. http://gerrit.cloudera.org:8080/#/c/14177/25/src/kudu/master/auto_rebalancer.cc File src/kudu/master/auto_rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/14177/25/src/kudu/master/auto_rebalancer.cc@189 PS25, Line 189: // With the current synchronous implementation, verify that any moves : // scheduled in the previous iteration completed. : DCHECK_EQ(0, replica_moves.size()); nit: with current state of the code, it seems this DCHECK_EQ() and the comment both belong to line 237, no? http://gerrit.cloudera.org:8080/#/c/14177/25/src/kudu/master/auto_rebalancer.cc@427 PS25, Line 427: communciate communicate http://gerrit.cloudera.org:8080/#/c/14177/25/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/14177/25/src/kudu/master/catalog_manager.cc@810 PS25, Line 810: PREDICT_TRUE Remove PREDICT_TRUE: the default value for --auto_rebalancing_enabled is 'false', so the is not much sense adding adding it here. Also, it's not a hot path at all: Init() is called once in a process's lifecycle. http://gerrit.cloudera.org:8080/#/c/14177/25/src/kudu/master/master_runner.cc File src/kudu/master/master_runner.cc: http://gerrit.cloudera.org:8080/#/c/14177/25/src/kudu/master/master_runner.cc@60 PS25, Line 60: // Validates that if auto-rebalancing is enabled, the cluster uses 3-4-3 replication : // (the --raft_prepare_replacement_before_eviction flag must be set to true). : static Status Validate343SchemeEnabledForAutoRebalancing() { : if (FLAGS_auto_rebalancing_enabled && : !FLAGS_raft_prepare_replacement_before_eviction) { : return Status::ConfigurationError("If enabling auto-rebalancing, " : "Kudu must be configured with " : " --raft_prepare_replacement_before_eviction."); : } : return Status::OK(); : } Usually this kind of precondition would be enforced via GROUP_FLAG_VALIDATOR (you can see an example at https://github.com/apache/kudu/blob/33fdea56517463b9c61b51b2618059f5ec8e0fda/src/kudu/master/catalog_manager.cc#L318-L327). If there is an issue with using GROUP_FLAG_VALIDATOR in this case (e.g., it's not possible with ValidateHiveMetastoreSaslEnabled() above as its comment describes), could you please articulate that in the comment? If ending up replacing with flag validator, consider moving the validator code and the instantiation of the validator into the file where --auto_rebalancing_enabled flag is defined. http://gerrit.cloudera.org:8080/#/c/14177/25/src/kudu/master/ts_descriptor.h File src/kudu/master/ts_descriptor.h: http://gerrit.cloudera.org:8080/#/c/14177/25/src/kudu/master/ts_descriptor.h@43 PS25, Line 43: class HostPort; No longer needed? -- To view, visit http://gerrit.cloudera.org:8080/14177 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifca25d1063c07047cf2123e6792b3c7395be20e4 Gerrit-Change-Number: 14177 Gerrit-PatchSet: 25 Gerrit-Owner: Hannah Nguyen <hannah.ngu...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Hannah Nguyen <hannah.ngu...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 17 Mar 2020 23:33:33 +0000 Gerrit-HasComments: Yes
