-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44994/#review124308
-----------------------------------------------------------



Thanks for the test! Main high level comments are to pull out the validation 
test into the previous review that adds validation and put it alongside other 
validation tests (master_validation_tests.cpp). Second thing was to put an 
expectation on the dispatch to Slave::shutdownExecutorTimeout rather than on 
the test containerizer, which should eliminate the need for your previous 
change.


src/tests/slave_tests.cpp (line 3186)
<https://reviews.apache.org/r/44994/#comment186777>

    No need for the NOTE IMO, but if you really think this needs calling out, 
the reader doesn't care about why the flag-less version of StartMaster isn't 
used; the reader cares perhaps about what the flags are used for. But as I read 
this I can quickly check what the flags are used for, and there are no suprises.
    
    A NOTE is typically used to raise awareness of a potential suprise, or to a 
subtlety that may be missed, both of which I can't see myself experiencing 
here, which is good! :)
    
    If we were to add a NOTE here you could imagine NOTEs all over the test 
(why do we use a mock executor? why do we use a test containerizer? why do we 
use agent flags? etc).



src/tests/slave_tests.cpp (lines 3198 - 3201)
<https://reviews.apache.org/r/44994/#comment186779>

    Is there precedent for calling these kinds of variables "agent"? Let's keep 
it consistent:
    
    ```
    $ grep -R 'Try<Owned<cluster::Slave>> slave' src | wc -l
         395
    $ grep -R 'Try<Owned<cluster::Slave>> agent' src | wc -l
           0
    ```



src/tests/slave_tests.cpp (lines 3207 - 3210)
<https://reviews.apache.org/r/44994/#comment186781>

    Use FutureArg and assert that when you use it later it is ready:
    
    ```
    Future<FrameworkID> frameworkId;
    EXPECT_CALL(sched, registered(&driver, _, _))
      .WillOnce(FutureArg<1>(&frameworkId));
    ```



src/tests/slave_tests.cpp (lines 3212 - 3251)
<https://reviews.apache.org/r/44994/#comment186782>

    Can you pull this up into a small test inside 
`master_validation_tests.cpp`? We've been trying to contain all of the tests 
for master/validation.hpp in there.



src/tests/slave_tests.cpp (lines 3249 - 3250)
<https://reviews.apache.org/r/44994/#comment186791>

    use the -> operator instead of .get().



src/tests/slave_tests.cpp (lines 3253 - 3298)
<https://reviews.apache.org/r/44994/#comment186792>

    It seems unintuitive that you made this a scope separate from the one 
below, the scope here says that it should override the default from the agent 
flag, but that is not tested in this scope, it is tested below. Can you merge 
the scopes? Actually, once we pull out the invalid duration test, we won't need 
any nested scoping here?



src/tests/slave_tests.cpp (line 3264)
<https://reviews.apache.org/r/44994/#comment186787>

    You don't need a settle here, AWAIT_READY will settle if the clock is 
paused.



src/tests/slave_tests.cpp (line 3267)
<https://reviews.apache.org/r/44994/#comment186789>

    Can you do a sweep to replace .get(). with the -> operator in these patches?



src/tests/slave_tests.cpp (lines 3304 - 3307)
<https://reviews.apache.org/r/44994/#comment186794>

    Thanks for this NOTE :)



src/tests/slave_tests.cpp (lines 3309 - 3312)
<https://reviews.apache.org/r/44994/#comment186796>

    This was a bit confusing, I don't need to know about `_wait` here (and it's 
likely to become a stale comment) could you consolidate this into the NOTE 
above? I think we could be a bit more succinct here, for example:
    
    ```
    // Note that the test containerizer only accepts "local" executors
    // and it considers them "terminated" only once destroy is called.
    ```



src/tests/slave_tests.cpp (lines 3317 - 3323)
<https://reviews.apache.org/r/44994/#comment186786>

    It seems fine to expect this but arguably if what we care about in this 
test is **how** the agent chooses the shutdown timeout, a FUTURE_DISPATCH on 
Slave::shutdownExecutorTimeout seems sufficient and closer reflects what is 
being tested? This avoids the need for the previous changes to the 
TestContainerizer AFAICT?



src/tests/slave_tests.cpp (line 3329)
<https://reviews.apache.org/r/44994/#comment186797>

    Might as well expect this alongside the expectation of 'status', can you 
add a FutureSatisfy here?



src/tests/slave_tests.cpp (line 3331)
<https://reviews.apache.org/r/44994/#comment186798>

    Would be nice to say that we have to spoof this because there isn't support 
in the driver for shutting down executors.



src/tests/slave_tests.cpp (lines 3337 - 3338)
<https://reviews.apache.org/r/44994/#comment186800>

    Would be helpful to say that we ensure the message is received by the agent 
before we start the timer. Have you run this in repetition?



src/tests/slave_tests.cpp (lines 3341 - 3342)
<https://reviews.apache.org/r/44994/#comment186803>

    Assuming we use the shutdown timeout dispatch expectation instead:
    
    ```
    // The shutdown timeout should not have fired, since the
    // ExecutorInfo contains a larger grace period.
    ```



src/tests/slave_tests.cpp (lines 3345 - 3347)
<https://reviews.apache.org/r/44994/#comment186805>

    ```
    // Trigger the shutdown grace period from the ExecutorInfo
    // (note that is is 2x the agent flag).
    ```



src/tests/slave_tests.cpp (lines 3353 - 3354)
<https://reviews.apache.org/r/44994/#comment186806>

    Remember that we have the -> operator now :)



src/tests/slave_tests.cpp (line 3357)
<https://reviews.apache.org/r/44994/#comment186808>

    Why do you resume?


- Ben Mahler


On March 18, 2016, 5:13 p.m., Alexander Rukletsov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44994/
> -----------------------------------------------------------
> 
> (Updated March 18, 2016, 5:13 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4949
>     https://issues.apache.org/jira/browse/MESOS-4949
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   src/tests/slave_tests.cpp ea1d776077bf638885db8421194aa4427c772169 
> 
> Diff: https://reviews.apache.org/r/44994/diff/
> 
> 
> Testing
> -------
> 
> On Mac OS 10.10.4:
> `make check`
> `GLOG_v=2 GTEST_FILTER="*SlaveTest*" ./bin/mesos-tests.sh --gtest_repeat=100 
> --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Reply via email to