----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18601/#review37084 -----------------------------------------------------------
Ship it! Looking good. Just a few minor tweaks. src/launcher/executor.cpp <https://reviews.apache.org/r/18601/#comment68375> Shouldn't need the default argv=NULL in both CommandExecutor and CommandExecutorProcess. Pick one entry point for the default value (probably the public-facing CommandExecutor), and make the value required elsewhere (CommandExecutorProcess). src/launcher/executor.cpp <https://reviews.apache.org/r/18601/#comment68372> Could be task.command() or argv, right? Perhaps these should be merged earlier so you don't have to check both each time task.command() is referenced. src/launcher/executor.cpp <https://reviews.apache.org/r/18601/#comment68373> argv will always trump a provided task.command()? This should be documented somewhere. And if both are provided, perhaps we should warn that task.command() is being overridden by argv. src/launcher/executor.cpp <https://reviews.apache.org/r/18601/#comment68381> Just reminding myself how pointer arithmetic works, please correct me if I'm interpreting this incorrectly. argv is an array of char*, i.e. char* argv[]. Incrementing a char** means increment the char** by sizeof(char*), which is the same size as any other pointer, essentially moving to the next element of the char* array. So, "argv+1" is the same as ((char**)(&argv[1])), meaning that you want to drop the first element of the argv array and pass argv[1...argc-1] on to the CommandExecutor. - Adam B On March 13, 2014, 11:40 a.m., Niklas Nielsen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18601/ > ----------------------------------------------------------- > > (Updated March 13, 2014, 11:40 a.m.) > > > Review request for mesos and Ben Mahler. > > > Repository: mesos-git > > > Description > ------- > > To ease using the mesos-executor to launch certain containers (see > https://issues.apache.org/jira/browse/MESOS-816) > we added support for passing and overwriting the command to launch from the > command line. > > > Diffs > ----- > > src/launcher/executor.cpp e30d77a > > Diff: https://reviews.apache.org/r/18601/diff/ > > > Testing > ------- > > Usual behavior of mesos-executor doesn't change. This is used and has been > tested with https://github.com/mesosphere/deimos. > > > Thanks, > > Niklas Nielsen > >
