Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14111 )
Change subject: KUDU-2069 pt 1: add a maintenance mode ...................................................................... Patch Set 2: (13 comments) A first quick pass. Will send an update soon. http://gerrit.cloudera.org:8080/#/c/14111/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14111/2//COMMIT_MSG@10 PS2, Line 10: failures of T will not be considered when determining : whether a given tablet is under-replicated I'm curious whether it mandates to have some sort of special response error code for CreateTable when it's not enough tablet servers to place tablet replicas because tablet servers are put into maintenance mode. Probably, I need to re-read the design doc. http://gerrit.cloudera.org:8080/#/c/14111/2//COMMIT_MSG@23 PS2, Line 23: whitelist nit: it sounds like rather a "blackgrey/list" or "the usual suspects". Just kidding, but maybe it makes sense to call it some other way :) http://gerrit.cloudera.org:8080/#/c/14111/2/src/kudu/master/maintenance_state-test.cc File src/kudu/master/maintenance_state-test.cc: http://gerrit.cloudera.org:8080/#/c/14111/2/src/kudu/master/maintenance_state-test.cc@84 PS2, Line 84: afiliated affiliated http://gerrit.cloudera.org:8080/#/c/14111/2/src/kudu/master/maintenance_state-test.cc@100 PS2, Line 100: void TearDown() override { : mini_master_->Shutdown(); : KuduTest::TearDown(); : } nit: move it up to be just after SetUp() -- that would look a bit better from the readability standpoint http://gerrit.cloudera.org:8080/#/c/14111/2/src/kudu/master/maintenance_state-test.cc@170 PS2, Line 170: and that it persists through restarts looks like an incomplete sentence http://gerrit.cloudera.org:8080/#/c/14111/2/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/14111/2/src/kudu/master/master.proto@816 PS2, Line 816: optional string tserver_uuid = 2; Maybe, it makes sense to allow setting the maintenance state in batches? http://gerrit.cloudera.org:8080/#/c/14111/2/src/kudu/master/master_service.cc File src/kudu/master/master_service.cc: http://gerrit.cloudera.org:8080/#/c/14111/2/src/kudu/master/master_service.cc@282 PS2, Line 282: if (!req->tablet_report().is_incremental()) { : ts_desc->UpdateNeedsFullTabletReport(false); : } Could you document the reasoning behind this code? http://gerrit.cloudera.org:8080/#/c/14111/2/src/kudu/master/ts_descriptor.h File src/kudu/master/ts_descriptor.h: http://gerrit.cloudera.org:8080/#/c/14111/2/src/kudu/master/ts_descriptor.h@113 PS2, Line 113: boost::optional<MaintenanceState>& Why boost::optional? Would simple enum suffice? enum MaintenanceState { kRegularMode, // or kNone kMaintenanceMode, }; http://gerrit.cloudera.org:8080/#/c/14111/2/src/kudu/master/ts_descriptor.h@207 PS2, Line 207: needs needs to send ? http://gerrit.cloudera.org:8080/#/c/14111/2/src/kudu/master/ts_descriptor.h@207 PS2, Line 207: or not nit: drop http://gerrit.cloudera.org:8080/#/c/14111/2/src/kudu/master/ts_manager.h File src/kudu/master/ts_manager.h: http://gerrit.cloudera.org:8080/#/c/14111/2/src/kudu/master/ts_manager.h@90 PS2, Line 90: it them ? http://gerrit.cloudera.org:8080/#/c/14111/2/src/kudu/master/ts_manager.h@132 PS2, Line 132: unregistered_maintenance_states_ nit: could you follow the same naming notation that was used for 'servers_by_id_'? http://gerrit.cloudera.org:8080/#/c/14111/2/src/kudu/master/ts_manager.cc File src/kudu/master/ts_manager.cc: http://gerrit.cloudera.org:8080/#/c/14111/2/src/kudu/master/ts_manager.cc@68 PS2, Line 68: string MaintenanceStateAsString(const boost::optional<MaintenanceState>& state) { : if (!state) { : return "NONE"; : } : switch (*state) { : case kMaintenanceMode: : return "MAINTENANCE MODE"; : } : return Substitute("UNKNOWN STATE ($0)", *state); : } nit: I think this method can return const reference to strings, returning simply "UNKNOWN STATE" if it gets some unexpected state (plus DCHECK on that). -- To view, visit http://gerrit.cloudera.org:8080/14111 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia857668b87560cdd451c2e7f90d72f8217ba5a4b Gerrit-Change-Number: 14111 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 23 Aug 2019 02:03:17 +0000 Gerrit-HasComments: Yes
