> 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. > > Nishant Suneja wrote: > 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."
Hey Timothy Since this patch became a little too big, I have broken down this patch into 2 separate patches: 29718 and 29720. - Nishant ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29437/#review66990 ----------------------------------------------------------- On Jan. 8, 2015, 7:54 a.m., Nishant Suneja wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29437/ > ----------------------------------------------------------- > > (Updated Jan. 8, 2015, 7:54 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 fd1c1aba0aa62372ab399bee5709ce81b8e92cec > src/slave/constants.cpp 2a99b1105af0e2d62b956fa96988f477937a46bd > src/slave/flags.hpp 670997dc3a702cd5edf33f2e5824c5e4dfe4ecef > src/slave/slave.hpp 70bd8c1fde4ea09fa54c76aa93424a1adb0309f6 > src/slave/slave.cpp 50b57819b55bdcdb9f49f20648199badc4d3f37b > src/tests/composing_containerizer_tests.cpp > 5ab5a36cadb7f8622bad0c5814e9a5fb338753ad > src/tests/containerizer.hpp 24b014f44d9eec56840e18cf39fbf9100f2c0711 > src/tests/slave_tests.cpp c50cbc799d4793243f184f9fe628b69a020adc66 > > Diff: https://reviews.apache.org/r/29437/diff/ > > > Testing > ------- > > Added the unit test : SlaveTest::ExecutorRegistrationTimeoutTrigger > Added the unit test : SlaveTest:: ExecutorLaunchTimeoutTrigger > Added the unit test : SlaveTest:: ExecutorNoLaunchTimeoutTrigger > make check succeeds. > > > Thanks, > > Nishant Suneja > >
