> On Nov. 20, 2014, 12:39 p.m., Niklas Nielsen wrote: > > src/tests/slave_tests.cpp, lines 1407-1408 > > <https://reviews.apache.org/r/28069/diff/2/?file=765866#file765866line1407> > > > > Please align like we do here: > > https://github.com/apache/mesos/blob/master/src/tests/slave_tests.cpp#L803 > > > > Here and below. > > Alexander Rukletsov wrote: > I would argue that this approach is more readable. Since it's expicitly > allowed by #3 of our C++ style guide, I would prefer to keep it as it is.
It leaves more void space in-front of "slave::..." I still root for wrapping as I suggested. BenH, we chatted about getting rid of wrapping rule 3 in http://mesos.apache.org/documentation/latest/mesos-c++-style-guide/. Should we do something about it? Can follow up with a doc patch. > On Nov. 20, 2014, 12:39 p.m., Niklas Nielsen wrote: > > src/launcher/executor.cpp, lines 687-690 > > <https://reviews.apache.org/r/28069/diff/2/?file=765859#file765859line687> > > > > Can you remind me again why you needed to move this? > > Alexander Rukletsov wrote: > So that we first read ENV vars and then cmd params. That is a what and not why :) It reads better to me, having the path code closer to it's use (which in this case is the executor constructor) so if you are not using the path variable in your timeout code, why move it up? - Niklas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28069/#review62277 ----------------------------------------------------------- On Nov. 24, 2014, 9:19 a.m., Alexander Rukletsov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28069/ > ----------------------------------------------------------- > > (Updated Nov. 24, 2014, 9:19 a.m.) > > > Review request for mesos, Ben Mahler, Niklas Nielsen, and Till Toenshoff. > > > Bugs: MESOS-1571 > https://issues.apache.org/jira/browse/MESOS-1571 > > > Repository: mesos-git > > > Description > ------- > > The configurable slave's executor_shutdown_grace_period flag is propagated to > Executor and CommandExecutor via CommandInfo and an environment variable. > Shutdown timeout in ExecutorProcess and signal escalation timeout in > CommandExecutor are now dependent on this value. > > > Diffs > ----- > > src/exec/exec.cpp e15f834 > src/launcher/executor.cpp 6175bf5 > src/slave/constants.hpp fd1c1ab > src/slave/constants.cpp 2a99b11 > src/slave/containerizer/containerizer.cpp 1448bea > src/slave/flags.hpp 4f5b8b4 > src/slave/slave.cpp ed63ded > src/tests/gc_tests.cpp cb5dc5f > src/tests/slave_tests.cpp 3b80ca9 > > Diff: https://reviews.apache.org/r/28069/diff/ > > > Testing > ------- > > make check (Mac OS 10.9.4, Ubuntu 14.04) > > > Thanks, > > Alexander Rukletsov > >
