> On March 15, 2016, 10:25 p.m., Ben Mahler wrote: > > src/launcher/executor.cpp, lines 121-125 > > <https://reviews.apache.org/r/44657/diff/4/?file=1299823#file1299823line121> > > > > Ditto from previous review comments, could you adjust the comment and > > logic to reflect that it's not a default anymore?
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, 10:25 p.m., Ben Mahler wrote: > > src/launcher/executor.cpp, lines 474-475 > > <https://reviews.apache.org/r/44657/diff/4/?file=1299823#file1299823line474> > > > > Could you be more explicit about what this guarantee is? i.e. > > mentioning that the agent, when determining the executor's shtudown grace > > period, will choose it based on the kill policy. > > > > Also, it seems surprising that killTask calls the version of shutdown > > here that doesn't take the time, why are we looking at the shutdown grace > > period if only a killTask was issued? I see why we pick the smaller if a > > shutdown was issued, but if only a killTask was issued, it seems we can > > ignore the shutdown period? I think we can go even further and simplify this code. Let me try to explain why. Command executor has a single task, hence `shutdown` and `killTask` calls are logically similar, hence exectuor shutdown grace period and kill task grace period should depend on each other. `ExecutorInfo.shutdown_grace_period` cannot be explicitly set by the user / framework for command executor. If the user omits kill policy, `ExecutorInfo.shutdown_grace_period` takes the agent default and is the only grace period available in the executor code. If the user sets kill policy, `ExecutorInfo.shutdown_grace_period` is calculated based on this value and though both grace periods are available in the executor code, we can use any. All these is not relevant for pre 0.28 agents, any value is fine this case. I would suggest to remove `min()` and *always* use `killPolicy` if set and fallback to shutdown grace period otherwise. Does it make sense? > On March 15, 2016, 10:25 p.m., Ben Mahler wrote: > > src/launcher/executor.cpp, line 499 > > <https://reviews.apache.org/r/44657/diff/4/?file=1299823#file1299823line499> > > > > I'd expect that the buffer is in addition to dealing with the reap > > interval, since the reap interval is not really a buffer but a time that we > > are expecting to wait. Could you add maybe an additional 1s buffer on top > > of the reap interval? > > > > Note that we could improve the reaper to block auxiliary threads when > > the process is a child, rather than polling for children. I had a TODO for > > this: > > > > ``` > > // TODO(bmahler): This can be optimized to use a thread per pid, where > > // each thread makes a blocking call to waitpid. This eliminates the > > // unfortunate poll delay. > > ``` I think refactoring the reaper is a great idea but maybe not now. I will add an extra time buffer and leave a todo to remove it once the reaper is updated. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44657/#review123765 ----------------------------------------------------------- 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/44657/ > ----------------------------------------------------------- > > (Updated March 15, 2016, 4:04 p.m.) > > > Review request for mesos, Ben Mahler and Gilbert Song. > > > Bugs: MESOS-4909 > https://issues.apache.org/jira/browse/MESOS-4909 > > > Repository: mesos > > > Description > ------- > > The command executor determines how much time it allots the > underlying task to clean up (effectively how long to wait for > the task to comply to SIGTERM before sending SIGKILL) based > on both optional task's `KillPolicy` and optional > `shutdown_grace_period` field in `ExecutorInfo`. > > > Diffs > ----- > > src/launcher/executor.cpp 4149f084b0b234fb995df04e9d1ca704feec082e > src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 > > Diff: https://reviews.apache.org/r/44657/diff/ > > > Testing > ------- > > The complete chain was tested. See https://reviews.apache.org/r/44662/. > > > Thanks, > > Alexander Rukletsov > >
