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

Reply via email to