Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16530 )
Change subject: [tool] KUDU-2181 CLI to add master ...................................................................... Patch Set 2: Code-Review+1 (5 comments) 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: > It's required else the RETURN_NOT_OK macro considers the template parameter Ah thanks for the pointer! Kind of surprised that we need to provide template parameters at all -- would've assumed they could be inferred given the arguments. Guess not. http://gerrit.cloudera.org:8080/#/c/16530/2/src/kudu/tools/tool_action_master.cc File src/kudu/tools/tool_action_master.cc: http://gerrit.cloudera.org:8080/#/c/16530/2/src/kudu/tools/tool_action_master.cc@150 PS2, Line 150: MonoDelta::FromSeconds(4); Do you think it's worth making this configurable as a tool argument? http://gerrit.cloudera.org:8080/#/c/16530/2/src/kudu/tools/tool_action_master.cc@166 PS2, Line 166: o & nit: flip ampersand order 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 typically avoid putting URLs directly in messages. In a similar vein, I wonder if it makes sense to just refer to this as "the official multi-master migration documentation" or somesuch. Same elsewhere 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 just log this message and return OK? Typically error messages can be scary for unsuspecting users, so where we can soften the tone while still being clear what next steps are, we should. That said, I imagine the error code makes it much easier to automate the full end-to-end workflow, so maybe not. In that automation, will we be able to differentiate between IncompleteErrors and other kinds of errors, and act accordingly? -- 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: 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 02:13:14 +0000 Gerrit-HasComments: Yes
