Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16530 )
Change subject: [tool] KUDU-2181 CLI to add master ...................................................................... Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/16530/2/src/kudu/tools/tool_action_master.cc File src/kudu/tools/tool_action_master.cc: PS2: > I added tests that utilize this CLI in the dynamic_multi_master-test.cc. Cool -- I guess dynamic_multi_master-test.cc is a good place to test functionality. However, we do usually add a tst into kudu-tool-test.cc for every new sub-tool: just to make sure we have coverage for the list of the tools. http://gerrit.cloudera.org:8080/#/c/16530/2/src/kudu/tools/tool_action_master.cc@196 PS2, Line 196: d master $0 to the cluster. Please fol > Like you mention, I want to avoid putting URLs in error messages as they ca Alternatively, consider the option of the tool printing the instructions. The tool would be self-sufficient in that case. It could be done if running `kudu master add --print-instructions` I guess the nice thing is that most of the kudu CLI commands involved could be exact ones, so a user could just copy-and-paste them :) http://gerrit.cloudera.org:8080/#/c/16530/3/src/kudu/tools/tool_action_master.cc File src/kudu/tools/tool_action_master.cc: http://gerrit.cloudera.org:8080/#/c/16530/3/src/kudu/tools/tool_action_master.cc@62 PS3, Line 62: 4 I'm curious: how did you come up with 4 seconds as the threshold? Does it include any extra margin to look stable enough even for larger catalog table changes and slower network? http://gerrit.cloudera.org:8080/#/c/16530/3/src/kudu/tools/tool_action_master.cc@159 PS3, Line 159: RETURN_NOT_OK((proxy.SyncRpc<ListMastersRequestPB, ListMastersResponsePB>( : req, &resp, "ListMasters", &MasterServiceProxy::ListMastersAsync))); : : if (resp.has_error()) { : return StatusFromPB(resp.error().status()); : } I'm curious what's the path forward if this returned an error due to some transient issue (leadership change, master restart/crash, etc.). Will it work as expected if re-running the tool? Will calling AddMaster for the second time with the same host:port succeed? http://gerrit.cloudera.org:8080/#/c/16530/3/src/kudu/tools/tool_action_master.cc@187 PS3, Line 187: return Status::NotFound(Substitute("New master $0 not found. Retry adding the master", : hp.ToString())); Can it ever happen if we are past the line 139 where AddMaster returned success? -- 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: 3 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: Thu, 04 Feb 2021 18:19:42 +0000 Gerrit-HasComments: Yes
