Hannah Nguyen has posted comments on this change. ( http://gerrit.cloudera.org:8080/14177 )
Change subject: KUDU-2780: create thread for auto-rebalancing ...................................................................... Patch Set 25: (10 comments) 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 comm Done http://gerrit.cloudera.org:8080/#/c/14177/25/src/kudu/master/auto_rebalancer.cc@245 PS25, Line 245: moves_scheduled_this_round_for_test_ = replica_moves.size(); > nit: maybe move this after we actually schedule the moves? ie after Execute Done http://gerrit.cloudera.org:8080/#/c/14177/25/src/kudu/master/auto_rebalancer.cc@247 PS25, Line 247: ExecuteMoves(replica_moves), > It's only OK for this to be WARN_NOT_OK because any moves that we failed to Will add a TODO for this, thank you! http://gerrit.cloudera.org:8080/#/c/14177/25/src/kudu/master/auto_rebalancer.cc@248 PS25, Line 248: "could not schedule auto-rebalancing replica moves"); > nit: this might be clearer as "failed to send move request" or somesuch. Done http://gerrit.cloudera.org:8080/#/c/14177/25/src/kudu/master/auto_rebalancer.cc@256 PS25, Line 256: "could not perform replica move"); > nit: this might be clearer as "failed to check if move completed" or somesu Done http://gerrit.cloudera.org:8080/#/c/14177/25/src/kudu/master/auto_rebalancer.cc@427 PS25, Line 427: communciate > communicate Done http://gerrit.cloudera.org:8080/#/c/14177/25/src/kudu/master/auto_rebalancer.cc@656 PS25, Line 656: Status move_completion_status; > nit: this is unused Done 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 'f Done 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_VALIDATO Created a validator in master/catalog_manager.cc where the flag is defined, thank you for the code pointer! 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? Done -- 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: Thu, 19 Mar 2020 23:15:27 +0000 Gerrit-HasComments: Yes
