----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44657/#review123765 -----------------------------------------------------------
src/launcher/executor.cpp (lines 121 - 125) <https://reviews.apache.org/r/44657/#comment186009> Ditto from previous review comments, could you adjust the comment and logic to reflect that it's not a default anymore? src/launcher/executor.cpp (lines 474 - 475) <https://reviews.apache.org/r/44657/#comment186011> 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? src/launcher/executor.cpp (line 498) <https://reviews.apache.org/r/44657/#comment186022> Hm.. it's unfortuate we returned an Option<T> for this overload of min, it should have been just a T. Oh well. src/launcher/executor.cpp (line 499) <https://reviews.apache.org/r/44657/#comment186021> 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. ``` src/launcher/executor.cpp (line 667) <https://reviews.apache.org/r/44657/#comment186023> Extra `<<` on this line src/launcher/executor.cpp (lines 931 - 937) <https://reviews.apache.org/r/44657/#comment186026> Hm.. how about: ``` // Get executor shutdown grace period from the environment. // // Note that we avoided introducing a command executor flag // for this because the command executor crashes if it sees // an unknown flag. This makes it difficult to add command // executor flags that are unconditionally set by the agent. ``` src/launcher/executor.cpp (line 943) <https://reviews.apache.org/r/44657/#comment186025> Please put the trailing space on the next line, so: ``` cerr << "Failed to parse value '" << value.get() << "'" << " of 'MESOS_EXECUTOR_SHUTDOWN_GRACE_PERIOD': " << parse.error(); ``` We've found this style makes it clearer that no spaces were forgotten, if you think about it, it's not the previous line that should be responsible for adding a space, it is the line that adds more information that is responsible for spacing itself from the previously added information. - 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/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 > >
