----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36425/#review92219 -----------------------------------------------------------
src/master/main.cpp (line 139) <https://reviews.apache.org/r/36425/#comment146238> We've tried to name our flags the same as how you'd see them on the command line, so s/ipDiscoveryScript/ip_discovery_script/ here please. Also, what about s/script/command/ here? Since we might not be executing a script here and it's more inline with the fact that we'll be executing this via the shell (i.e., it's a shell command, as opposed to say doing s/script/executable/ which might imply we're going to fork/exec). src/master/main.cpp (line 144) <https://reviews.apache.org/r/36425/#comment146239> Rather than overriding the setting any reason not to error out to be more friendly to an operator? Seems like this would save someone some time debugging in the future and would be as simple as: if (ip.isSome() && ip_discovery_script.isSome()) { // Print nice error message and maybe usage and exit. } src/master/main.cpp (lines 181 - 209) <https://reviews.apache.org/r/36425/#comment146240> Apologies for not giving you this feedback sooner, but we already have a wrapper for `popen` with `os::shell`. It's used a handful of places in the code instead of `popen`: ostringstream out; Try<int> status = os::shell(&out, ip_discovery_script.get()); if (status.isError()) { EXIT(EXIT_FAILURE) << ...; } else if (WIFSIGNALED(status.get())) { EXIT(EXIT_FAILURE) << ... << strsignal(WTERMSIG(status.get())); } else if (WEXITSTATUS(status.get()) != EXIT_SUCCESS) { EXIT(EXIT_FAILURE) << ... << stringify (WEXITSTATUS(status.get())); } const string ipAddress = strings::trim(out.get()); LOG(INFO) << ...; os::setenv("LIBPROCESS_IP", ipAddress); src/master/main.cpp (line 184) <https://reviews.apache.org/r/36425/#comment146242> What is process::executeCommand? Did you mean process::subprocess here? src/master/main.cpp (line 208) <https://reviews.apache.org/r/36425/#comment146241> We've tried to consistently wrap things like IPs in single quotes in string messages. - Benjamin Hindman On July 13, 2015, 9:35 p.m., Marco Massenzio wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36425/ > ----------------------------------------------------------- > > (Updated July 13, 2015, 9:35 p.m.) > > > Review request for mesos, Benjamin Hindman and Cody Maloney. > > > Bugs: MESOS-2902 > https://issues.apache.org/jira/browse/MESOS-2902 > > > Repository: mesos > > > Description > ------- > > Jira: MESOS-2902 > > It is sometimes useful to enable an external script to > configure the IP address the Mesos Master will bind to > on the server, where it's not desirable to set the > --ip flag and/or a "wrapper" script is not a viable option. > > This patch adds a --ip_discovery_script to point to a local > script that will emit as its only output the IP address that > the Master will bind to: only spaces and newlines are allowed; > further, as we cannot use the `libprocess` sub-processing > facilities, we cannot timeout the script, should this block > for long times (or even forever). > > This will override the --ip flag, which, even if set, will be > ignored. > > > Diffs > ----- > > docs/configuration.md feee5594c88112f77ce382cb3dd8628653f92d01 > src/master/main.cpp fd4de4d0d9c3e9617408022d10b5e161bdc911e1 > > Diff: https://reviews.apache.org/r/36425/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Marco Massenzio > >
