Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16530 )
Change subject: [tool] KUDU-2181 CLI to add master ...................................................................... Patch Set 2: (10 comments) http://gerrit.cloudera.org:8080/#/c/16530/2/src/kudu/client/master_proxy_rpc.cc File src/kudu/client/master_proxy_rpc.cc: http://gerrit.cloudera.org:8080/#/c/16530/2/src/kudu/client/master_proxy_rpc.cc@288 PS2, Line 288: template class AsyncLeaderMasterRpc<AddMasterRequestPB, AddMasterResponsePB>; : template class AsyncLeaderMasterRpc<ChangeTServerStateRequestPB, ChangeTServerStateResponsePB>; : template class AsyncLeaderMasterRpc<CreateTableRequestPB, CreateTableResponsePB>; : template class AsyncLeaderMasterRpc<IsCreateTableDoneRequestPB, IsCreateTableDoneResponsePB>; : template class AsyncLeaderMasterRpc<DeleteTableRequestPB, DeleteTableResponsePB>; : template class AsyncLeaderMasterRpc<AlterTableRequestPB, AlterTableResponsePB>; : template class AsyncLeaderMasterRpc<IsAlterTableDoneRequestPB, IsAlterTableDoneResponsePB>; : template class AsyncLeaderMasterRpc<GetTableSchemaRequestPB, GetTableSchemaResponsePB>; : template class AsyncLeaderMasterRpc<GetTableLocationsRequestPB, GetTableLocationsResponsePB>; : template class AsyncLeaderMasterRpc<GetTabletLocationsRequestPB, GetTabletLocationsResponsePB>; : template class AsyncLeaderMasterRpc<GetTableStatisticsRequestPB, GetTableStatisticsResponsePB>; > nit: if adding this in alphabetical order, it would be great to re-sort the Done http://gerrit.cloudera.org:8080/#/c/16530/2/src/kudu/master/dynamic_multi_master-test.cc File src/kudu/master/dynamic_multi_master-test.cc: http://gerrit.cloudera.org:8080/#/c/16530/2/src/kudu/master/dynamic_multi_master-test.cc@369 PS2, Line 369: RETURN_NOT_OK(cluster_->GetLeaderMasterIndex(&leader_master_idx)); > nit: extra spaces to start the line. Done http://gerrit.cloudera.org:8080/#/c/16530/2/src/kudu/tools/tool_action_common.cc File src/kudu/tools/tool_action_common.cc: http://gerrit.cloudera.org:8080/#/c/16530/2/src/kudu/tools/tool_action_common.cc@926 PS2, Line 926: std::vector<uint32_t> required_feature_flags) { > warning: the parameter 'required_feature_flags' is copied for each invocati I'm ignoring this comment as it's part of coding style guidelines to pass by value and move instead of using rvalue reference or const reference. http://gerrit.cloudera.org:8080/#/c/16530/1/src/kudu/tools/tool_action_master.cc File src/kudu/tools/tool_action_master.cc: http://gerrit.cloudera.org:8080/#/c/16530/1/src/kudu/tools/tool_action_master.cc@138 PS1, Line 138: > Ah thanks for the pointer! Kind of surprised that we need to provide templa I tried without the explicit template parameters but compiler complains failing to match function. http://gerrit.cloudera.org:8080/#/c/16530/2/src/kudu/tools/tool_action_master.cc File src/kudu/tools/tool_action_master.cc: PS2: > If adding new tools, don't forget to update kudu-tool-test.cc to add a sign I added tests that utilize this CLI in the dynamic_multi_master-test.cc. http://gerrit.cloudera.org:8080/#/c/16530/2/src/kudu/tools/tool_action_master.cc@150 PS2, Line 150: MonoDelta::FromSeconds(4); > +1: there is --timeout flag for other CLI tools Done http://gerrit.cloudera.org:8080/#/c/16530/2/src/kudu/tools/tool_action_master.cc@166 PS2, Line 166: o & > nit: flip ampersand order Done http://gerrit.cloudera.org:8080/#/c/16530/2/src/kudu/tools/tool_action_master.cc@196 PS2, Line 196: \"Migrating to Multiple Kudu Masters\" > nit: in the past we've linked to sites that have gone stale, so we typicall Like you mention, I want to avoid putting URLs in error messages as they can change and hence the title of the documentation procedure. As a user I find myself lost whenever error messages vaguely mention "official documentation on so and so". It's unlikely we'll change the title of the procedure even if we move sites and putting the title of the procedure helps find the documentation quickly with a simple internet search. So the only risk is possible change in title of the procedure in future which can be mitigated by adding a comment in the documentation page preventing users from accidentally changing title without updating the error message string. For current users even with change in site, they'll be able to find the relevant documentation more easily. http://gerrit.cloudera.org:8080/#/c/16530/2/src/kudu/tools/tool_action_master.cc@203 PS2, Line 203: Status::Incomplete( : Substitute("New master $0 part of the Raft configuration; role: $1, member_type: $2. " : "Please follow the next steps which includes system catalog tablet copy, " : "updating master addresses etc. from the Kudu administration documentation on " : "\"Migrating to Multiple Kudu Masters\".", : hp.ToString(), master_role, master_type)); > Given this is somewhat a common workflow, would it make sense to instead ju Good point. I'll change to Status::OK(). Changing tests/automation is not much of a problem. http://gerrit.cloudera.org:8080/#/c/16530/2/src/kudu/tools/tool_action_master.cc@517 PS2, Line 517: add_master > Maybe, just 'add' ? The context sub-item is already 'master'. Compare Done -- To view, visit http://gerrit.cloudera.org:8080/16530 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I507f301d1aba17327eb35728eed0d765e86ef4cc Gerrit-Change-Number: 16530 Gerrit-PatchSet: 2 Gerrit-Owner: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 02 Feb 2021 23:12:22 +0000 Gerrit-HasComments: Yes
