> On Dec. 26, 2014, 9:11 p.m., Timothy Chen wrote:
> > Can you add a unit test?
>
> Nishant Suneja wrote:
> I wanted to add one, but I would need access to the "Future" object for
> the container launch, or "Timer" object for registration timeout trigger,
> both of which
> I cannot access, since they are local variables, and NOT the member
> variables of any class.
>
> Any ideas as to how do I get around it, without introducting any extra
> member variables ?
>
> Timothy Chen wrote:
> you can get hold of the future by passing in a mock containerizzer and
> perform a EXPECT_CALL(comtainerizer, launch(.....)) and return a future that
> you instantiate in test.
>
> Nishant Suneja wrote:
> Thats a neat way to get hold of the future object. So, for testing I have
> to basically verify that the timer object for Slave's "registerExecutor" is
> not created, till we reach the READY state of the future object. Now, I could
> have done something crazy like iterate the map of timer objects in the Clock
> class, and verify that we dont have this timer object till we are in READY
> state of the future object.
> However, this map is a defined as static in clock.cpp, so I wouldnt be
> able to access it in the testcase, without making the map global (not a good
> idea).
>
> So, again, any ideas ?
>
> Timothy Chen wrote:
> The easiest way is to simply advance the clock (Clock::advance(....))
> until the timeout, and verify that the delay call is not yet called. Then you
> satisfy the future and advance the time again and verify it is called.
>
> Nishant Suneja wrote:
> Ok...And whats the best way to verify that the
> "Slave::registerExecutorTimeout" has been called (after advancing the clock).
> I could check the state of the executor, and verify that its TERMINATING,
> which would mean that this method has executed.
>
> Any other cleaner way to verify this callback ? I believe that
> EXPECT_CALL macro only works for classes with MOCK_METHOD set of methods
> defined. So, I cant use that.
>
> Timothy Chen wrote:
> One way to verify this is to use FUTURE_DISPATCH (you can search for
> examples in existing test). FUTURE_DISPATCH returns a future, which becomes
> ready when the method provided is dispatched (which means queued to serve in
> libprocess). Delay shouldn't queue it until the timeout happen.
>
> Nishant Suneja wrote:
> Yeah.. FUTURE_DISPATCH should do the job.
> However, a question about the failure testcase. When we invoke
> Clock::advance() by a certain duration, any registered timer object which
> falls within that duration gets
> called. For 2nd test, I can use the AWAIT_READY() macro on the future,
> and then advancing the clock should result in our "registerExecutorTimeout"
> being called, which is good.
>
> Although, for the 1st test case, before advancing the clock, I would want
> to make sure that the launch() function has actually returned, and our
> current code triggers the timer (otherwise, I would be advancing the clock,
> when no timer object has actually been set, so obviously nothing will get
> fired by advancing the clock).
> EXPECT_CALL gives us a handle to that Future object, but how do I
> determine that launch() method has returned (still in pending state though),
> so i can advance the clock ? i.e. on what condition should I wait here?
>
> Timothy Chen wrote:
> EXPECT_CALL is called when the method is invoked, and whatever action you
> chain it with (.WillOnce(Returns(....)) is called immediately, so for example
> if you want to wait on the call, you can use EXPECT_CALL(containerizer,
> launch(_, _, _......)).WillOnce(FutureSatisify(&future)) and then
> AWAIT_READY(future), which the EXPECT_CALL will set the future state to be
> ready when the launch method is called.
>
> I think what I would do though is to create a promise, and return that
> promise's future instead:
>
> Promise<Nothing> promise;
> EXPECT_CALL(containerizer, launch(_, _, _,....))
> .WillOnce(Return(promise.future()));
>
> And then Clock::pause(), Clock::advance(timeout), and verify that the
> executor timeout is not ready ASSERT_TRUE(timeoutFuture.isPending())
>
> then satisfy the promise so the future is set to ready:
>
> promise.set(Nothing())
>
> After wards advance the clock and verify that it is ready.
Hey Timothy
So, I went through the documentation of gmock, and got a better understanding
of the magic MACROS.
That said, I still have 2 questions:
1) During a unit test run, where we instantiate a master and a slave, how many
processes are we actually dealing with ?
Logic says that we have 1 test case process, and its 2 child processes
(slave and master).
I ask this because it seems that we are sharing the containerizer object
between test case processs and its child (slave) process.
Also, the callback in the ACTION part of the EXPECT_CALL, I believe that its
been invoked in the context of the slave process.
Just wanted to clarify if I am on the correct page.
2) So, if the above statement is correct, in the code below, we seem to have a
race. We would expect this test case to pass with our
new code. However, in my patch, the call to delay() happens in onReady()
method of the launch future object, which happens
AFTER invocation of containerizer::launch().
Now, its very well possible, that we pause and advance the clock in the
parent test case process, before the delay()
gets called in the slave process, in which case the timer wont fire at all.
3) Which brings me to my third question, as to are we sharing the same clock
across these 3 processes ?
Clock class (for eg.) is packaged as part of libprocess library, which can
be shared among these 3 processes,
but there would be separate copies of static/global data of this class for
the 3 processes (only the code segment should be shared).
So, will the timer set by one process, be visible to another process at all ?
CODE:
Future<Nothing> launch;
EXPECT_CALL(containerizer, launch(_, _, _, _, _, _, _))
.WillOnce(DoAll(FutureSatisfy(&launch),
Return(true)));
//future object for call to "registerExecutorTimeout"
Future<Nothing> executorRegistrationTimeout =
FUTURE_DISPATCH(_, &Slave::registerExecutorTimeout);
driver.launchTasks(offers.get()[0].id(), tasks);
AWAIT_READY(launch);
Clock::pause();
Clock::advance(flags.executor_registration_timeout);
//ensure that the "registerExecutorTimeout" HAS fired
ASSERT_TRUE(executorRegistrationTimeout.isReady());
Clock::resume();
- Nishant
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29437/#review66171
-----------------------------------------------------------
On Dec. 26, 2014, 6:29 p.m., Nishant Suneja wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29437/
> -----------------------------------------------------------
>
> (Updated Dec. 26, 2014, 6:29 p.m.)
>
>
> Review request for mesos.
>
>
> 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 onReady() 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.hpp 70bd8c1fde4ea09fa54c76aa93424a1adb0309f6
> src/slave/slave.cpp 50b57819b55bdcdb9f49f20648199badc4d3f37b
>
> Diff: https://reviews.apache.org/r/29437/diff/
>
>
> Testing
> -------
>
> make check succeeds.
>
>
> Thanks,
>
> Nishant Suneja
>
>
