----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49473/#review140505 -----------------------------------------------------------
Fix it, then Ship it! LGTM! Just find some nits not from your patch. src/slave/containerizer/mesos/launch.cpp (lines 166 - 170) <https://reviews.apache.org/r/49473/#comment205906> Not yours, since you are on it, could we fix the indentation? ``` while ((length = os::read(pipe[0], &dummy, sizeof(dummy))) == -1 && errno == EINTR); ``` src/slave/containerizer/mesos/launch.cpp (lines 372 - 373) <https://reviews.apache.org/r/49473/#comment205908> Also not yours. Could we fix the style here? Seems like from https://github.com/apache/mesos/commit/81e893d5cdb8a5873dd0c61ad7eceaa323aff466 src/slave/containerizer/mesos/launch.cpp (line 375) <https://reviews.apache.org/r/49473/#comment205909> Should we add a TODO here? Consider to use `execvpe`. - Gilbert Song On June 30, 2016, 2:22 p.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49473/ > ----------------------------------------------------------- > > (Updated June 30, 2016, 2:22 p.m.) > > > Review request for mesos, Benjamin Mahler, Gilbert Song, and Ian Downes. > > > Bugs: MESOS-5753 > https://issues.apache.org/jira/browse/MESOS-5753 > > > Repository: mesos > > > Description > ------- > > This is the first step allowing the command executor to directly use > `mesos-containerizer launch` to launch user task. The control pipe is > used to synchronize with the parent process. In the command executor, > this synchronization is not needed. Therefore, we make it optional. > > > Diffs > ----- > > src/slave/containerizer/mesos/launch.cpp > 83f4d7f28c066a605aa84862eca9fde900ec96c6 > > Diff: https://reviews.apache.org/r/49473/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jie Yu > >
