> On March 15, 2016, 11:21 p.m., Ben Mahler wrote:
> > src/docker/executor.cpp, lines 101-105
> > <https://reviews.apache.org/r/44660/diff/4/?file=1299829#file1299829line101>
> >
> >     Ditto feedback from previous reviews about this no longer being a 
> > default.

Per offline discussion, we decided to drop this code altogether. The env var 
has the actual value. If the env var is not set, the agent is pre 0.28 and it 
doesn't support custom shutdown grace periods anyway, so even if ExecutorInfo 
specifies the custom grace period, we can ignore it.


> On March 15, 2016, 11:21 p.m., Ben Mahler wrote:
> > src/docker/executor.cpp, lines 235-239
> > <https://reviews.apache.org/r/44660/diff/4/?file=1299829#file1299829line235>
> >
> >     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 :)

I'm a bit hesitant to add the comment here about how `docker->run()` works. 
Such comments tend to become outdated when the internal implementation changes.


> On March 15, 2016, 11:21 p.m., Ben Mahler wrote:
> > src/docker/executor.cpp, lines 657-663
> > <https://reviews.apache.org/r/44660/diff/4/?file=1299829#file1299829line657>
> >
> >     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.

Yes, that would be ideal. However, we'll learn about kill policy only later, 
when the task is launched. The case when the user sets both the docker stop 
timeout and the killpolicy, and the kill policy is smaller than the docker flag 
seems to be rare to me. Moreover, in such case we give *not less* time than 
requested. I'd suggest to leave this as is as opposed to saving the docker flag 
value and deciding later whether we need it or not.


- Alexander


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44660/#review123782
-----------------------------------------------------------


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
> 
>

Reply via email to