----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29437/#review66447 -----------------------------------------------------------
src/slave/slave.cpp <https://reviews.apache.org/r/29437/#comment109997> We usually don't comment what the code logic is doing, as it's self explanatory here. What we usually put in the comments is why, can you put down the reason why we start the timer after the launch future is set? src/tests/slave_tests.cpp <https://reviews.apache.org/r/29437/#comment109998> It's unclear what asynchronous here means. It seems originally the delay(timeout) is happening async already, it's just when the delay is actually scheduled. I think just mentioning that the test is checking the timeout happens after the containerizer launch future completes, and remove the Asynchronous in the command and the test name. src/tests/slave_tests.cpp <https://reviews.apache.org/r/29437/#comment110001> We are starting to move away from the space between ending brackets. So please remove the extra space: Try<PID<Master>> src/tests/slave_tests.cpp <https://reviews.apache.org/r/29437/#comment110000> About commenting style, all comments needs to end with period, within 70 char width and have a space in the beginning. src/tests/slave_tests.cpp <https://reviews.apache.org/r/29437/#comment110002> There should be only one wait call as well, let's use WillOnce to verify the right behavior. src/tests/slave_tests.cpp <https://reviews.apache.org/r/29437/#comment109999> Recover should only get called once right? (.WillOnce) src/tests/slave_tests.cpp <https://reviews.apache.org/r/29437/#comment110003> Fix comment style mentioned above. src/tests/slave_tests.cpp <https://reviews.apache.org/r/29437/#comment110004> Fix comment style mentioned above. src/tests/slave_tests.cpp <https://reviews.apache.org/r/29437/#comment110005> Fix comment style mentioned above, and everywhere else. src/tests/slave_tests.cpp <https://reviews.apache.org/r/29437/#comment110006> We typically name the future the same name as the function, AWAIT_READY(launch). src/tests/slave_tests.cpp <https://reviews.apache.org/r/29437/#comment110007> Why settle here? seems unncessary. src/tests/slave_tests.cpp <https://reviews.apache.org/r/29437/#comment110008> Why settle? src/tests/slave_tests.cpp <https://reviews.apache.org/r/29437/#comment110009> Seems pretty self-explanatory? Have you looked at all the other tests that refers to executor_registration_timeout, and made sure they all passed as well? Since timing is a pretty sensitive for mesos test, I suggest you also try to run all the tests on repeat (./bin/mesos-tests.sh --gtest_break_on_failure --gtest_repeat=100 --verbose) and make sure all tests still pass. - Timothy Chen On Dec. 31, 2014, 5:11 a.m., Nishant Suneja wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29437/ > ----------------------------------------------------------- > > (Updated Dec. 31, 2014, 5:11 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 asychronously, > when the onAny() callback is made for the container's future object, instead > of starting the > timer synchronously when the launchExecutor() method of the Framework class > is invoked. > > > Diffs > ----- > > src/slave/slave.cpp 50b57819b55bdcdb9f49f20648199badc4d3f37b > src/tests/composing_containerizer_tests.cpp > 5ab5a36cadb7f8622bad0c5814e9a5fb338753ad > src/tests/containerizer.hpp 24b014f44d9eec56840e18cf39fbf9100f2c0711 > src/tests/slave_tests.cpp f2896a1fc4521452e29fd261a6f117372345dcfc > > Diff: https://reviews.apache.org/r/29437/diff/ > > > Testing > ------- > > Added the unit test : > SlaveTest::AsynchronousExecutorRegistrationTimeoutTrigger > make check succeeds. > > > Thanks, > > Nishant Suneja > >
