> On March 15, 2016, 6:39 p.m., Ben Mahler wrote:
> > src/exec/exec.cpp, line 113
> > <https://reviews.apache.org/r/44654/diff/4/?file=1299793#file1299793line113>
> >
> > If you'd like to add the `private` qualifier, why isn't `kill` left as
> > protected?
>
> Alexander Rukletsov wrote:
> I do not know what is our general guideline here. I think we are
> inconsitent about access modifiers; I've checked `AwaitProcess`,
> `CollectProcess`, `ReaperProcess`, `SequenceProcess`. Here I followed this
> sentence from the Google style guide: "Limit the use of protected to those
> member functions that might need to be accessed from subclasses. Note that
> data members should be private." I left `kill` as protected because that was
> the original intention of the author.
>
> I'm fine with making it private, because it's unlikely we are going to
> subclass it. However, it will be inconsistent to what we have in
> `ShutdownProcess` in "executor/executor.cpp".
>
> Moreover, I think we should factor out `ShutdownProcess` because it is
> already used in two places. I suggest to leave the code consistent for now
> and change to private when we refactor. Does it make sense?
I'd rather not factor out ShutdownProcess because it is not clearly a good
re-usable abstraction. Both uses could be better served by the following very
simple code, no need for sharing:
```
delay(gracePeriod,
[]() {
killpg(0, SIGKILL);
os::sleep(Seconds(5));
exit(EXIT_FAILURE);
});
```
There isn't really a benefit to the ShutdownProcess abstraction on top of just
doing a simple delay. It is necessary only because there isn't support for
executing the delay within a synchronous or `__executor__`-based execution
context (to avoid the ExecutorProcess blocking issue).
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44654/#review123713
-----------------------------------------------------------
On March 17, 2016, 11:30 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44654/
> -----------------------------------------------------------
>
> (Updated March 17, 2016, 11:30 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-4911
> https://issues.apache.org/jira/browse/MESOS-4911
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Executor shutdown grace period, which configured on the agent, is
> propagated to executors via the `MESOS_EXECUTOR_SHUTDOWN_GRACE_PERIOD`
> environment variable. The executor library uses this timeout to delay
> the hard shutdown of the related executor.
>
>
> Diffs
> -----
>
> src/exec/exec.cpp 741786132f3a8cc43f5b9ced262429038832a946
> src/executor/executor.cpp 87db4e02cbaa778aab0173741bfe066fdee9a48d
>
> Diff: https://reviews.apache.org/r/44654/diff/
>
>
> Testing
> -------
>
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
