> On Jan. 7, 2015, 7:15 a.m., Timothy Chen wrote: > > Hi Nishant, thanks for the updated review. Just FYI, our preferred patches > > are actually small and logical commits. So in your example, is one patch > > just for executor registration timeout, and another review for executor > > launch timeout. And you can depend one review on top of another. The larger > > the patch the harder it is to accept and commit it.
Thanks for the update Timothy. I will keep that in mind from the next patch onwards. Also, any thoughs on my earlier comment, in case you missed it: "Yeah.. I have implemented that. So, I was writing a testcase to verify the container launch timeout logic, which has 2 conditions to be tested:: a) container launch fails, and timeout action triggers. b) container launch happens, and timeout action is cancelled. I wanted to test both of them in the same testcase. However, once the "launchExecutor()" call has been made for the default executor testing the first case, 2nd case doesnt call the "launchExecutor()" because the framework already has an in-memory pointer for it, and skips the launch. What would be the easiest way to trigger launchExecutor() the 2nd time ? I wanted to get a handle of the Framework structure, and call destoryExecutor(), before launching the tasks again, but couldnt find an easy way to get the handle." - Nishant ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29437/#review66990 ----------------------------------------------------------- On Jan. 7, 2015, 6:41 a.m., Nishant Suneja wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29437/ > ----------------------------------------------------------- > > (Updated Jan. 7, 2015, 6:41 a.m.) > > > Review request for mesos and Timothy Chen. > > > Bugs: MESOS-999 > https://issues.apache.org/jira/browse/MESOS-999 > > > Repository: mesos-git > > > Description > ------- > > As part of this bug fix, I have trigerred the executor registration timeout > timer after the container's future object is set, instead of starting the > timer when the container launch is still pending. > > Also, a new executor launch timer has been added. This timer gates the time > in which a successful executor container launch should happen. The executor > registration timer starts after the successful container launch. > > > Diffs > ----- > > src/slave/constants.hpp fd1c1ab > src/slave/constants.cpp 2a99b11 > src/slave/flags.hpp 670997d > src/slave/slave.hpp 70bd8c1 > src/slave/slave.cpp 50b5781 > src/tests/composing_containerizer_tests.cpp 5ab5a36 > src/tests/containerizer.hpp 24b014f > src/tests/slave_tests.cpp f2896a1 > > Diff: https://reviews.apache.org/r/29437/diff/ > > > Testing > ------- > > Added the unit test : SlaveTest::ExecutorRegistrationTimeoutTrigger > Added the unit test : SlaveTest:: ExecutorLaunchTimeoutTrigger > make check succeeds. > > > Thanks, > > Nishant Suneja > >
