----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44427/#review128534 -----------------------------------------------------------
src/slave/slave.cpp (lines 5857 - 5870) <https://reviews.apache.org/r/44427/#comment191970> 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? - Vinod Kone On April 9, 2016, 8:47 a.m., Qian Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44427/ > ----------------------------------------------------------- > > (Updated April 9, 2016, 8:47 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 f090c853b8affc4be5eecb4f616ec881fc2b60c3 > > Diff: https://reviews.apache.org/r/44427/diff/ > > > Testing > ------- > > > Thanks, > > Qian Zhang > >
