----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44660/#review123782 -----------------------------------------------------------
Looks pretty good, mostly similar comments from the command executor side, with a few exceptions. src/docker/executor.cpp (lines 101 - 105) <https://reviews.apache.org/r/44660/#comment186032> Ditto feedback from previous reviews about this no longer being a default. src/docker/executor.cpp (lines 206 - 209) <https://reviews.apache.org/r/44660/#comment186040> Hm.. why are we trying to cope with the container getting destroyed? This is killTask, not shutdown, so I don't see why we need to worry about the buffer here. src/docker/executor.cpp (lines 232 - 233) <https://reviews.apache.org/r/44660/#comment186036> Chose the 'docker stop --timeout' based on ... src/docker/executor.cpp (lines 235 - 239) <https://reviews.apache.org/r/44660/#comment186046> Ditto earlier comments. I would expect the buffer to be in addition to the reap interval. It also wasn't obvious to me that docker->run uses the reaper internally, so perhaps a note here about that. At some point we'll make reaping children be non-polling :) src/docker/executor.cpp (lines 634 - 642) <https://reviews.apache.org/r/44660/#comment186033> Ditto the feedback from the previous review. src/docker/executor.cpp (lines 649 - 650) <https://reviews.apache.org/r/44660/#comment186034> ditto here, can you put the " of " on the next line? src/docker/executor.cpp (lines 657 - 663) <https://reviews.apache.org/r/44660/#comment186035> Hm.. ideally we could ignore the docker stop flag if a kill policy was set, because the user is being explicit about how much time they need. This is the same as how we ignore the executor shutdown grace period flag if the executor explicitly sets one. - Ben Mahler On March 15, 2016, 4:04 p.m., Alexander Rukletsov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44660/ > ----------------------------------------------------------- > > (Updated March 15, 2016, 4:04 p.m.) > > > Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen. > > > Bugs: MESOS-4909 > https://issues.apache.org/jira/browse/MESOS-4909 > > > Repository: mesos > > > Description > ------- > > The docker executor determines how much time it allots the > underlying container to clean up (via passing the timeout to > the docker daemon) based on both optional task's `KillPolicy` > and optional `shutdown_grace_period` field in `ExecutorInfo`. > > > Diffs > ----- > > src/docker/executor.cpp 1921d4a1ce3c45b4e2f81f0ef5914d5830da6866 > > Diff: https://reviews.apache.org/r/44660/diff/ > > > Testing > ------- > > The complete chain was tested. See https://reviews.apache.org/r/44662/. > > > Thanks, > > Alexander Rukletsov > >
