----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43082/#review117472 -----------------------------------------------------------
src/launcher/executor.cpp (line 143) <https://reviews.apache.org/r/43082/#comment178710> Why 'executorLaunchCommand'? This is the task command, right? I would simply use 'command' for this naming here. src/launcher/executor.cpp (lines 143 - 164) <https://reviews.apache.org/r/43082/#comment178713> I would restructure the code here for better readability: ``` // Determine the command to launch the task. CommandInfo command; if (taskCommand.isSome()) { ... command = parse.get(); } else if (task.has_command()) { command = task.command(); } else { CHECK_SOME(override) << "Expecting task " << task.task_id() << " to have a command!"; } if (override.isNone()) { // TODO(jieyu): For now, ... if (command.shell()) { CHECK(command.has_value()) << "..."; } else { CHECK(command.has_value()) << "..."; } } ``` src/launcher/executor.cpp (line 179) <https://reviews.apache.org/r/43082/#comment178715> YOu forgot to update this! src/launcher/executor.cpp (line 277) <https://reviews.apache.org/r/43082/#comment178714> You may want to rename it to 'commandString'. src/launcher/executor.cpp (line 746) <https://reviews.apache.org/r/43082/#comment178677> Can you add a TODO here to deprecate this flag since no one is using it, and it'll cause confusing to your newly added flag. src/launcher/executor.cpp (lines 767 - 769) <https://reviews.apache.org/r/43082/#comment178707> If specified, this is the overrided command for launching the task (instead of the command from TaskInfo). - Jie Yu On Feb. 2, 2016, 5:18 p.m., Gilbert Song wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43082/ > ----------------------------------------------------------- > > (Updated Feb. 2, 2016, 5:18 p.m.) > > > Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen. > > > Bugs: MESOS-4004 > https://issues.apache.org/jira/browse/MESOS-4004 > > > Repository: mesos > > > Description > ------- > > Added new flag to command executor for command passing. > > > Diffs > ----- > > src/launcher/executor.cpp 356d311fdf97b2c4663c60e13ede7cdb71a264c7 > > Diff: https://reviews.apache.org/r/43082/diff/ > > > Testing > ------- > > make check (ubuntu14.04 + clang-3.6) > > > Thanks, > > Gilbert Song > >
