Re: Review Request 44994: Added a test for executor shutdown grace period.

[Alexander Rukletsov]

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

(Updated March 24, 2016, 4:15 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 (updated)
-

  src/slave/slave.hpp bc68cf1e82cf2b1a64b8c8a2f923ccf94a4cb229 
  src/tests/slave_tests.cpp 5f47a715b9ee6c9f552854a228aca376761f1155 

Diff: https://reviews.apache.org/r/44994/diff/


Testing
---

On Mac OS 10.10.4:
`make check`
`GTEST_FILTER="*ExecutorShutdownGracePeriod*" ./bin/mesos-tests.sh 
--gtest_repeat=100 --gtest_break_on_failure`


Thanks,

Alexander Rukletsov

Re: Review Request 44994: Added a test for executor shutdown grace period.

[Ben Mahler]

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


Fix it, then Ship it!





src/tests/slave_tests.cpp (line 3267)


s/TimedOut/Timeout/



src/tests/slave_tests.cpp (lines 3308 - 3309)


AWAIT_EXPECT_EQ


- Ben Mahler


On March 23, 2016, 11:22 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44994/
> ---
> 
> (Updated March 23, 2016, 11:22 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/slave/slave.hpp 7520cc356e2b1b7f5fff15f33071a46a7b05e762 
>   src/tests/slave_tests.cpp ea1d776077bf638885db8421194aa4427c772169 
> 
> Diff: https://reviews.apache.org/r/44994/diff/
> 
> 
> Testing
> ---
> 
> On Mac OS 10.10.4:
> `make check`
> `GTEST_FILTER="*ExecutorShutdownGracePeriod*" ./bin/mesos-tests.sh 
> --gtest_repeat=100 --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44994: Added a test for executor shutdown grace period.

[Alexander Rukletsov]

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

(Updated March 23, 2016, 11:22 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 (updated)
-

  src/slave/slave.hpp 7520cc356e2b1b7f5fff15f33071a46a7b05e762 
  src/tests/slave_tests.cpp ea1d776077bf638885db8421194aa4427c772169 

Diff: https://reviews.apache.org/r/44994/diff/


Testing
---

On Mac OS 10.10.4:
`make check`
`GTEST_FILTER="*ExecutorShutdownGracePeriod*" ./bin/mesos-tests.sh 
--gtest_repeat=100 --gtest_break_on_failure`


Thanks,

Alexander Rukletsov

Re: Review Request 44994: Added a test for executor shutdown grace period.

[Alexander Rukletsov]

> On March 18, 2016, 11:24 p.m., Ben Mahler wrote:
> > src/tests/slave_tests.cpp, line 3264
> > 
> >
> > You don't need a settle here, AWAIT_READY will settle if the clock is 
> > paused.
> 
> Alexander Rukletsov wrote:
> I feel that this is not obvious, that settle will happen in `AWAIT_READY` 
> if clock is paused. Moreover, when we don't await, we have to explicitly 
> settle, which—I think—makes it harder for folks to follow the test and reason 
> when settle is necessary, because sometimes a necessary settle is implicit 
> (inside await). Hence I tend to put explicit settles when I feel this makes 
> the code more obvious. Do you have a strong preference?
> 
> Ben Mahler wrote:
> Think about it this way, if you put a `settle()` before the 
> `AWAIT_READY`, then the implication is that you don't need `AWAIT_READY` at 
> all, you could just do `ASSERT_TRUE(f.isReady())` because you've done the 
> necessary settling explicitly. This is why it seems strange to have a 
> `settle` immediately preceed an `AWAIT_READY`, its the job of `AWAIT_READY` 
> to wait until the future is ready (which means doing the right thing when the 
> clock is paused, otherwise we would have to write a lot of `settle` calls).

I see, makes sense. Thanks Ben!


- Alexander


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


On March 22, 2016, 5:07 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44994/
> ---
> 
> (Updated March 22, 2016, 5:07 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/slave/slave.hpp 7520cc356e2b1b7f5fff15f33071a46a7b05e762 
>   src/tests/slave_tests.cpp ea1d776077bf638885db8421194aa4427c772169 
> 
> Diff: https://reviews.apache.org/r/44994/diff/
> 
> 
> Testing
> ---
> 
> On Mac OS 10.10.4:
> `make check`
> `GTEST_FILTER="*ExecutorShutdownGracePeriod*" ./bin/mesos-tests.sh 
> --gtest_repeat=100 --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44994: Added a test for executor shutdown grace period.

[Alexander Rukletsov]

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

(Updated March 22, 2016, 5:07 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 (updated)
-

  src/slave/slave.hpp 7520cc356e2b1b7f5fff15f33071a46a7b05e762 
  src/tests/slave_tests.cpp ea1d776077bf638885db8421194aa4427c772169 

Diff: https://reviews.apache.org/r/44994/diff/


Testing
---

On Mac OS 10.10.4:
`make check`
`GTEST_FILTER="*ExecutorShutdownGracePeriod*" ./bin/mesos-tests.sh 
--gtest_repeat=100 --gtest_break_on_failure`


Thanks,

Alexander Rukletsov

Re: Review Request 44994: Added a test for executor shutdown grace period.

[Alexander Rukletsov]

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

(Updated March 21, 2016, 6:33 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 (updated)
---

On Mac OS 10.10.4:
`make check`
`GTEST_FILTER="*ExecutorShutdownGracePeriod*" ./bin/mesos-tests.sh 
--gtest_repeat=100 --gtest_break_on_failure`


Thanks,

Alexander Rukletsov

Re: Review Request 44994: Added a test for executor shutdown grace period.

[Alexander Rukletsov]

> On March 18, 2016, 11:24 p.m., Ben Mahler wrote:
> > src/tests/slave_tests.cpp, lines 3317-3323
> > 
> >
> > 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?

I agree it is cleaner and better expresses the intention. But for that, we 
should promote `Slave::shutdownExecutorTimeout()` to public. I was thinking to 
avoid this by updating the test containerizer. I'm with replacing it with a 
`FUTURE_DISPATCH` on `Slave::shutdownExecutorTimeout()`.


- Alexander


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


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
> 
>

Re: Review Request 44994: Added a test for executor shutdown grace period.

[Alexander Rukletsov]

> On March 21, 2016, 1 p.m., haosdent huang wrote:
> > src/tests/slave_tests.cpp, line 3214
> > 
> >
> > I notice in http://search-hadoop.com/m/0Vlr6J38NSZOXgd2 said we should 
> > not use negative durations. So this test would not necessary once we 
> > implement it?

We haven't reached consensus on whether we shoud make durations non-negative, 
hence until then this test does make sense. If we require durations be 
non-negative one day, this test should fail and we'll remove it altogether.


- Alexander


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


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
> 
>

Re: Review Request 44994: Added a test for executor shutdown grace period.

[haosdent huang]

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




src/tests/slave_tests.cpp (line 3214)


I notice in http://search-hadoop.com/m/0Vlr6J38NSZOXgd2 said we should not 
use negative durations. So this test would not necessary once we implement it?


- haosdent huang


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
> 
>

Re: Review Request 44994: Added a test for executor shutdown grace period.

[Alexander Rukletsov]

> On March 18, 2016, 11:24 p.m., Ben Mahler wrote:
> > src/tests/slave_tests.cpp, line 3264
> > 
> >
> > You don't need a settle here, AWAIT_READY will settle if the clock is 
> > paused.

I feel that this is not obvious, that settle will happen in `AWAIT_READY` if 
clock is paused. Moreover, when we don't await, we have to explicitly settle, 
which—I think—makes it harder for folks to follow the test and reason when 
settle is necessary, because sometimes a necessary settle is implicit (inside 
await). Hence I tend to put explicit settles when I feel this makes the code 
more obvious. Do you have a strong preference?


- Alexander


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


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
> 
>

Review Request 44994: Added a test for executor shutdown grace period.

[Alexander Rukletsov]

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

Review request for mesos and Ben Mahler.


Bugs: MESOS-4949
https://issues.apache.org/jira/browse/MESOS-4949


Repository: mesos


Description
---

Added a test for executor shutdown grace period.


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

Re: Review Request 44994: Added a test for executor shutdown grace period.

[Alexander Rukletsov]

---
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 (updated)
---

See summary.


Diffs (updated)
-

  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

Re: Review Request 44994: Added a test for executor shutdown grace period.

[Ben Mahler]

---
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)


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)


Is there precedent for calling these kinds of variables "agent"? Let's keep 
it consistent:

```
$ grep -R 'Try slave' src | wc -l
 395
$ grep -R 'Try agent' src | wc -l
   0
```



src/tests/slave_tests.cpp (lines 3207 - 3210)


Use FutureArg and assert that when you use it later it is ready:

```
Future frameworkId;
EXPECT_CALL(sched, registered(, _, _))
  .WillOnce(FutureArg<1>());
```



src/tests/slave_tests.cpp (lines 3212 - 3251)


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)


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



src/tests/slave_tests.cpp (lines 3253 - 3298)


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)


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



src/tests/slave_tests.cpp (line 3267)


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



src/tests/slave_tests.cpp (lines 3304 - 3307)


Thanks for this NOTE :)



src/tests/slave_tests.cpp (lines 3309 - 3312)


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)


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)


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



src/tests/slave_tests.cpp (line 3331)


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)


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)