> 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?
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? - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44654/#review123713 ----------------------------------------------------------- On March 15, 2016, 2:15 p.m., Alexander Rukletsov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44654/ > ----------------------------------------------------------- > > (Updated March 15, 2016, 2:15 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 > > Diff: https://reviews.apache.org/r/44654/diff/ > > > Testing > ------- > > The complete chain was tested. See https://reviews.apache.org/r/44662/. > > > Thanks, > > Alexander Rukletsov > >
