Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18124 )
Change subject: KUDU-2915: add tool to remove a tablet server ...................................................................... Patch Set 5: Code-Review+1 (12 comments) Thank you for adding this new CLI tool! http://gerrit.cloudera.org:8080/#/c/18124/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18124/5//COMMIT_MSG@9 PS5, Line 9: kudu tserver remove Should we call this 'kudu tserver unregister' instead? From what I can see, the server is simply unregistered from the system catalog, but if it checks in with the master, it will be simply re-registered back. http://gerrit.cloudera.org:8080/#/c/18124/5//COMMIT_MSG@11 PS5, Line 11: restart nit: restarting http://gerrit.cloudera.org:8080/#/c/18124/5//COMMIT_MSG@13 PS5, Line 13: could remove nit: removes http://gerrit.cloudera.org:8080/#/c/18124/5//COMMIT_MSG@14 PS5, Line 14: We could also force remove nit: It's also possible to remove ... http://gerrit.cloudera.org:8080/#/c/18124/5//COMMIT_MSG@15 PS5, Line 15: adding some : optional arguments nit: instead of referring those as 'some', maybe specify the exact names of the command-line arguments for achieving that? http://gerrit.cloudera.org:8080/#/c/18124/5/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/18124/5/src/kudu/master/master.proto@1074 PS5, Line 1074: persisted data nit: persistent catalog http://gerrit.cloudera.org:8080/#/c/18124/5/src/kudu/master/master.proto@1076 PS5, Line 1076: optional string uuid = 1; nit: even if it seems self-obvious, it would be nice to add a comment for this field as well http://gerrit.cloudera.org:8080/#/c/18124/5/src/kudu/master/master.proto@1080 PS5, Line 1080: optional bool remove_tserver_state = 2 I'm curious if it makes sense to enable this by default? Could you add an extra sentence to reason about the default setting for this field? http://gerrit.cloudera.org:8080/#/c/18124/5/src/kudu/master/master_service.cc File src/kudu/master/master_service.cc: http://gerrit.cloudera.org:8080/#/c/18124/5/src/kudu/master/master_service.cc@341 PS5, Line 341: could nit: could --> can (for simplicity, keep the same tense in the sentence: https://justpublishingadvice.com/maintaining-good-tense-control-in-your-writing/) http://gerrit.cloudera.org:8080/#/c/18124/5/src/kudu/master/master_service.cc@351 PS5, Line 351: RespondFailure If following the comment above, this should be RespondSuccess(), shouldn't it? http://gerrit.cloudera.org:8080/#/c/18124/5/src/kudu/master/master_service.cc@355 PS5, Line 355: if (remove_tserver_state && l.leader_status().ok()) { : s = server_->ts_manager()->SetTServerState(ts_uuid, : TServerStatePB::NONE, : ChangeTServerStateRequestPB::ALLOW_MISSING_TSERVER, : server_->catalog_manager()->sys_catalog()); : if (PREDICT_FALSE(!s.ok())) { : rpc->RespondFailure(s); : return; : } : } Shouldn't we first try removing tserver's state record? Are the system catalog and the run-time state consistent if RemoveTServer() passes, but SetTServerState() fails? http://gerrit.cloudera.org:8080/#/c/18124/5/src/kudu/master/ts_manager.cc File src/kudu/master/ts_manager.cc: http://gerrit.cloudera.org:8080/#/c/18124/5/src/kudu/master/ts_manager.cc@328 PS5, Line 328: --force_remove_live_tserver I'm not sure this is exactly relevant in the error message from the server given that the API can be used from elsewhere, not only from the newly introduced kudu CLI tool. -- To view, visit http://gerrit.cloudera.org:8080/18124 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If1f5c2979a8d14428f4bcc8e850c57ce228c793a Gerrit-Change-Number: 18124 Gerrit-PatchSet: 5 Gerrit-Owner: Yifan Zhang <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Wed, 19 Jan 2022 04:55:56 +0000 Gerrit-HasComments: Yes
