Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17267 )
Change subject: [master][tool] KUDU-2181 Tool to orchestrate adding a master ...................................................................... Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/17267/2/src/kudu/tools/tool_action_master.cc File src/kudu/tools/tool_action_master.cc: http://gerrit.cloudera.org:8080/#/c/17267/2/src/kudu/tools/tool_action_master.cc@172 PS2, Line 172: MonoTime deadline = MonoTime::Now() + MonoDelta::FromSeconds(FLAGS_wait_secs); : do { : Status wait_status = new_master->WaitNoBlock(); : if (!wait_status.IsTimedOut()) { : return Status::RuntimeError("Failed to bring up new master"); : } : if (is_catalog_mngr_running(proxy.get())) { : *new_master_out = std::move(new_master); : return Status::OK(); : } : SleepFor(MonoDelta::FromMilliseconds(100)); : } while (MonoTime::Now() < deadline); : : return Status::TimedOut("Timed out waiting for the new master to come up"); > Here and in general, if there's an error, should we kill the subprocess? Us I was relying on the destructor for the error cases but I guess it's better to be explicit. http://gerrit.cloudera.org:8080/#/c/17267/2/src/kudu/tools/tool_action_master.cc@349 PS2, Line 349: // Get the flags that'll be needed to bring up new master and for system catalog copy. : GFlagsMap flags_map = GetFlagsMap(); > Thinking about the typical CM-run Kudu deployment, there could be a few oth Btw we are passing through all the flags here to bring up the master specified by the user and only omitting the ones that are specific for this command (wait_secs). So the ones like block_manager will be passed through as well if the user specifies them and not the only ones listed in the help. I debated between passing a single argument wrapped under double quote using a delimiter (a) v/s this implementation (b) that uses the gflags optional parameters. Wrapping with double quotes is needed else it gets interpreted as a gflag for the command. For (a): - Need to decide on a delimiter. Comma, colon are out for obvious reasons. Space is not suitable too since a gflag could be passed as --flag val as well. Used pipe as the delimiter. - It's unusual and cumbersome for a user to pass such a parameter. "flag_a=val1|flag_b=va2,va3" etc. For (b): - One downside is possible collision in parameter. Currently we only have one "wait_secs" so it's easy to manage. - Looks intuitive just like kudu master run command. I actually implemented variant (a) and reverted to using (b) for reasons mentioned above. But I'm open if there are reasons in favor of (a). The 3rd option would be reading from a file but I didn't explore that much. http://gerrit.cloudera.org:8080/#/c/17267/2/src/kudu/tools/tool_action_master.cc@363 PS2, Line 363: [[maybe_unused]] > Hm, what might be unused here? underscore _ -- To view, visit http://gerrit.cloudera.org:8080/17267 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8f99cf2b3f1738c4127c7e7288beab61daf42e7b Gerrit-Change-Number: 17267 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-Comment-Date: Mon, 05 Apr 2021 18:50:05 +0000 Gerrit-HasComments: Yes
