> On April 12, 2016, 9:07 p.m., Vinod Kone wrote: > > src/slave/slave.cpp, lines 5857-5870 > > <https://reviews.apache.org/r/44427/diff/2/?file=1338099#file1338099line5857> > > > > I think this will break slave recovery when a slave is restarted with > > the flag set to true. This is because it will incorrectly set > > `commandExecutor` to be `false` for driver based command line executors. > > > > I think this code should be: > > > > ``` > > { > > CHECK_NOTNULL(slave); > > > > // See if this is driver based command executor. > > Result<string> executorPath = > > os::realpath(path::join(slave->flags.launcher_dir, > > "mesos-executor"); > > > > if (executorPath.isSome()) { > > commandExecutor = strings::contains(info.command.value(), > > executorPath.get()); > > } > > > > // See if this is HTTP based commmand executor. > > if (!commandExecutor) { > > executorPath = > > os::realpath(path::join(slave->flags.launcher_dir, > > "mesos-http-executor"); > > > > commandExecutor = strings::contains(info.command.value(), > > executorPath.get()); > > } > > > > return commandExecutor; > > } > > > > ``` > > > > Can you please write a test for this i.e., slave restart with flag > > change? > > Qian Zhang wrote: > Thanks for catching this! I will write a test for slave restarting with > flag change soon. > > Qian Zhang wrote: > BTW, I think I need to add a test for slave restarting with flag change > in `slave_recovery_tests.cpp`, right?
That's right. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44427/#review128534 ----------------------------------------------------------- On April 13, 2016, 9:14 a.m., Qian Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44427/ > ----------------------------------------------------------- > > (Updated April 13, 2016, 9:14 a.m.) > > > Review request for mesos, Anand Mazumdar and Vinod Kone. > > > Bugs: MESOS-3558 > https://issues.apache.org/jira/browse/MESOS-3558 > > > Repository: mesos > > > Description > ------- > > Added --http_command_executor flag. > > > Diffs > ----- > > docs/configuration.md ba00ec563c449345effb3114111812601addcfc2 > src/slave/flags.hpp 300db49100d989d6c0409766b1341cb956ea1631 > src/slave/flags.cpp dd7bc9a48dfd8481336a2d2ec0beecd19a342644 > src/slave/slave.cpp 6ee277d3c69d03a318c88b7dcb1abc14107ed3e2 > > Diff: https://reviews.apache.org/r/44427/diff/ > > > Testing > ------- > > > Thanks, > > Qian Zhang > >
