Re: Review Request 35438: Fixed fetcher cache test race for resource offers when starting tasks and changed corresponding CHECK to EXPECT.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [35438]

All tests passed.

- Mesos ReviewBot


On June 22, 2015, 2:57 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35438/
> ---
> 
> (Updated June 22, 2015, 2:57 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: 2815, 2831, 2857, and 2858
> https://issues.apache.org/jira/browse/2815
> https://issues.apache.org/jira/browse/2831
> https://issues.apache.org/jira/browse/2857
> https://issues.apache.org/jira/browse/2858
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Follow up to RR https://reviews.apache.org/r/35247/, which was not good 
> enough, fixed only one of two problems.
> 
> Using DeclineOffers() instead of Return() should make the master resend 
> offers so we can launch tasks. See line 205 below.
> 
> Following Jie's suggestion, no more CHECK_READY inside launchTask(s)(), but 
> we return a Try instead and follow call sites with EXPECT_SOME(task(s)).
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp 8bd5dd847fb189d0eeeaa760d3ec8ce3af1c2392 
> 
> Diff: https://reviews.apache.org/r/35438/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 35438: Fixed fetcher cache test race for resource offers when starting tasks and changed corresponding CHECK to EXPECT.

[Bernd Mathiske]

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

(Updated June 22, 2015, 7:57 a.m.)


Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen.


Changes
---

Reinserted bug numbers.


Bugs: 2815, 2831, 2857, and 2858
https://issues.apache.org/jira/browse/2815
https://issues.apache.org/jira/browse/2831
https://issues.apache.org/jira/browse/2857
https://issues.apache.org/jira/browse/2858


Repository: mesos


Description
---

Follow up to RR https://reviews.apache.org/r/35247/, which was not good enough, 
fixed only one of two problems.

Using DeclineOffers() instead of Return() should make the master resend offers 
so we can launch tasks. See line 205 below.

Following Jie's suggestion, no more CHECK_READY inside launchTask(s)(), but we 
return a Try instead and follow call sites with EXPECT_SOME(task(s)).


Diffs
-

  src/tests/fetcher_cache_tests.cpp 8bd5dd847fb189d0eeeaa760d3ec8ce3af1c2392 

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


Testing
---

make check


Thanks,

Bernd Mathiske

Re: Review Request 35438: Fixed fetcher cache test race for resource offers when starting tasks and changed corresponding CHECK to EXPECT.

[Bernd Mathiske]

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

(Updated June 22, 2015, 7:46 a.m.)


Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy 
Chen.


Changes
---

Fixed edits suggested by @tillt.


Repository: mesos


Description
---

Follow up to RR https://reviews.apache.org/r/35247/, which was not good enough, 
fixed only one of two problems.

Using DeclineOffers() instead of Return() should make the master resend offers 
so we can launch tasks. See line 205 below.

Following Jie's suggestion, no more CHECK_READY inside launchTask(s)(), but we 
return a Try instead and follow call sites with EXPECT_SOME(task(s)).


Diffs (updated)
-

  src/tests/fetcher_cache_tests.cpp 8bd5dd847fb189d0eeeaa760d3ec8ce3af1c2392 

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


Testing
---

make check


Thanks,

Bernd Mathiske

Re: Review Request 35438: Fixed fetcher cache test race for resource offers when starting tasks and changed corresponding CHECK to EXPECT.

[Till Toenshoff]

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



src/tests/fetcher_cache_tests.cpp (line 473)


s/"  +/" +/



src/tests/fetcher_cache_tests.cpp (line 1044)


s/Http/HTTP/g (in comments)


- Till Toenshoff


On June 22, 2015, 2:30 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35438/
> ---
> 
> (Updated June 22, 2015, 2:30 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and 
> Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Follow up to RR https://reviews.apache.org/r/35247/, which was not good 
> enough, fixed only one of two problems.
> 
> Using DeclineOffers() instead of Return() should make the master resend 
> offers so we can launch tasks. See line 205 below.
> 
> Following Jie's suggestion, no more CHECK_READY inside launchTask(s)(), but 
> we return a Try instead and follow call sites with EXPECT_SOME(task(s)).
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp 8bd5dd847fb189d0eeeaa760d3ec8ce3af1c2392 
> 
> Diff: https://reviews.apache.org/r/35438/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 35438: Fixed fetcher cache test race for resource offers when starting tasks and changed corresponding CHECK to EXPECT.

[Bernd Mathiske]

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

(Updated June 22, 2015, 7:30 a.m.)


Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy 
Chen.


Changes
---

Removed redundant error output at CHECK_SOME().


Repository: mesos


Description
---

Follow up to RR https://reviews.apache.org/r/35247/, which was not good enough, 
fixed only one of two problems.

Using DeclineOffers() instead of Return() should make the master resend offers 
so we can launch tasks. See line 205 below.

Following Jie's suggestion, no more CHECK_READY inside launchTask(s)(), but we 
return a Try instead and follow call sites with EXPECT_SOME(task(s)).


Diffs (updated)
-

  src/tests/fetcher_cache_tests.cpp 8bd5dd847fb189d0eeeaa760d3ec8ce3af1c2392 

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


Testing
---

make check


Thanks,

Bernd Mathiske

Re: Review Request 35438: Fixed fetcher cache test race for resource offers when starting tasks and changed corresponding CHECK to EXPECT.

[Benjamin Hindman]

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

Ship it!


This looks good Bernd. And you should be able to commit this on your own now. 
;-)


src/tests/fetcher_cache_tests.cpp (line 563)


EXPECT_SOME will print the error for you automagically, no need to do it 
yourself! Please fix all spots in this review, thanks!


- Benjamin Hindman


On June 15, 2015, 2:28 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35438/
> ---
> 
> (Updated June 15, 2015, 2:28 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Till Toenshoff, Timothy Chen, and 
> Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Follow up to RR https://reviews.apache.org/r/35247/, which was not good 
> enough, fixed only one of two problems.
> 
> Using DeclineOffers() instead of Return() should make the master resend 
> offers so we can launch tasks. See line 205 below.
> 
> Following Jie's suggestion, no more CHECK_READY inside launchTask(s)(), but 
> we return a Try instead and follow call sites with EXPECT_SOME(task(s)).
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp 8bd5dd847fb189d0eeeaa760d3ec8ce3af1c2392 
> 
> Diff: https://reviews.apache.org/r/35438/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 35438: Fixed fetcher cache test race for resource offers when starting tasks and changed corresponding CHECK to EXPECT.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [35438]

All tests passed.

- Mesos ReviewBot


On June 15, 2015, 2:28 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35438/
> ---
> 
> (Updated June 15, 2015, 2:28 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Till Toenshoff, Timothy Chen, and 
> Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Follow up to RR https://reviews.apache.org/r/35247/, which was not good 
> enough, fixed only one of two problems.
> 
> Using DeclineOffers() instead of Return() should make the master resend 
> offers so we can launch tasks. See line 205 below.
> 
> Following Jie's suggestion, no more CHECK_READY inside launchTask(s)(), but 
> we return a Try instead and follow call sites with EXPECT_SOME(task(s)).
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp 8bd5dd847fb189d0eeeaa760d3ec8ce3af1c2392 
> 
> Diff: https://reviews.apache.org/r/35438/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 35438: Fixed fetcher cache test race for resource offers when starting tasks and changed corresponding CHECK to EXPECT.

[Bernd Mathiske]

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

(Updated June 15, 2015, 7:28 a.m.)


Review request for mesos, Benjamin Hindman, Till Toenshoff, Timothy Chen, and 
Vinod Kone.


Changes
---

Addressed Till's review.


Repository: mesos


Description
---

Follow up to RR https://reviews.apache.org/r/35247/, which was not good enough, 
fixed only one of two problems.

Using DeclineOffers() instead of Return() should make the master resend offers 
so we can launch tasks. See line 205 below.

Following Jie's suggestion, no more CHECK_READY inside launchTask(s)(), but we 
return a Try instead and follow call sites with EXPECT_SOME(task(s)).


Diffs (updated)
-

  src/tests/fetcher_cache_tests.cpp 8bd5dd847fb189d0eeeaa760d3ec8ce3af1c2392 

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


Testing
---

make check


Thanks,

Bernd Mathiske

Re: Review Request 35438: Fixed fetcher cache test race for resource offers when starting tasks and changed corresponding CHECK to EXPECT.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [35438]

All tests passed.

- Mesos ReviewBot


On June 15, 2015, 8:52 a.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35438/
> ---
> 
> (Updated June 15, 2015, 8:52 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Till Toenshoff, Timothy Chen, and 
> Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Follow up to RR https://reviews.apache.org/r/35247/, which was not good 
> enough, fixed only one of two problems.
> 
> Using DeclineOffers() instead of Return() should make the master resend 
> offers so we can launch tasks. See line 205 below.
> 
> Following Jie's suggestion, no more CHECK_READY inside launchTask(s)(), but 
> we return a Try instead and follow call sites with EXPECT_SOME(task(s)).
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp 8bd5dd847fb189d0eeeaa760d3ec8ce3af1c2392 
> 
> Diff: https://reviews.apache.org/r/35438/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 35438: Fixed fetcher cache test race for resource offers when starting tasks and changed corresponding CHECK to EXPECT.

[Till Toenshoff]

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

Ship it!



src/tests/fetcher_cache_tests.cpp


Would it make sense here to add the underlying failure reason here any 
everywhere else, if available?

```
return Error("Failed to wait for resource offers: " + (offers.isFailed ? 
offers.failure() : "discarded"));
```



src/tests/fetcher_cache_tests.cpp


See above.



src/tests/fetcher_cache_tests.cpp


Can we get this comment into some more explicit form that also follows our 
styleguide?


- Till Toenshoff


On June 15, 2015, 8:52 a.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35438/
> ---
> 
> (Updated June 15, 2015, 8:52 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Till Toenshoff, Timothy Chen, and 
> Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Follow up to RR https://reviews.apache.org/r/35247/, which was not good 
> enough, fixed only one of two problems.
> 
> Using DeclineOffers() instead of Return() should make the master resend 
> offers so we can launch tasks. See line 205 below.
> 
> Following Jie's suggestion, no more CHECK_READY inside launchTask(s)(), but 
> we return a Try instead and follow call sites with EXPECT_SOME(task(s)).
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp 8bd5dd847fb189d0eeeaa760d3ec8ce3af1c2392 
> 
> Diff: https://reviews.apache.org/r/35438/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 35438: Fixed fetcher cache test race for resource offers when starting tasks and changed corresponding CHECK to EXPECT.

[Bernd Mathiske]

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

(Updated June 15, 2015, 1:51 a.m.)


Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy 
Chen.


Changes
---

Added TODO regarding the "15 seconds" naked constant when awaiting offers.


Repository: mesos


Description
---

Follow up to RR https://reviews.apache.org/r/35247/, which was not good enough, 
fixed only one of two problems.

Using DeclineOffers() instead of Return() should make the master resend offers 
so we can launch tasks. See line 205 below.

Following Jie's suggestion, no more CHECK_READY inside launchTask(s)(), but we 
return a Try instead and follow call sites with EXPECT_SOME(task(s)).


Diffs (updated)
-

  src/tests/fetcher_cache_tests.cpp 8bd5dd847fb189d0eeeaa760d3ec8ce3af1c2392 

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


Testing
---

make check


Thanks,

Bernd Mathiske

Re: Review Request 35438: Fixed fetcher cache test race for resource offers when starting tasks and changed corresponding CHECK to EXPECT.

[Bernd Mathiske]

> On June 14, 2015, 11:45 p.m., Timothy Chen wrote:
> > src/tests/fetcher_cache_tests.cpp, line 358
> > 
> >
> > Does this mean when the default changes we need to change this value 
> > too?
> > 
> > Also what's the rationale making it a Error/Try? Is this because a 
> > timeout can be a valid case here?

Since the default is a naked number in AWAIT_..., we would have to change 
library code, first, to improve this. I'll insert a TODO.

Many such test support macros are geared towards inlining all code into every 
test. Having local returns of type void, they don't work inside functions that 
do not return void - and they are incapable of ending the whole test from 
within nested calls.

So, in consequence, the strategy to uplevel errors into call level zero and use 
an EXPECT_... macro there seems right.

Another alternative is avoiding procedural abstraction, which is common in our 
(test) code base.


- Bernd


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


On June 14, 2015, 6:54 a.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35438/
> ---
> 
> (Updated June 14, 2015, 6:54 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Follow up to RR https://reviews.apache.org/r/35247/, which was not good 
> enough, fixed only one of two problems.
> 
> Using DeclineOffers() instead of Return() should make the master resend 
> offers so we can launch tasks. See line 205 below.
> 
> Following Jie's suggestion, no more CHECK_READY inside launchTask(s)(), but 
> we return a Try instead and follow call sites with EXPECT_SOME(task(s)).
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp 8bd5dd847fb189d0eeeaa760d3ec8ce3af1c2392 
> 
> Diff: https://reviews.apache.org/r/35438/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 35438: Fixed fetcher cache test race for resource offers when starting tasks and changed corresponding CHECK to EXPECT.

[Timothy Chen]

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



src/tests/fetcher_cache_tests.cpp


Does this mean when the default changes we need to change this value too?

Also what's the rationale making it a Error/Try? Is this because a timeout 
can be a valid case here?


- Timothy Chen


On June 14, 2015, 1:54 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35438/
> ---
> 
> (Updated June 14, 2015, 1:54 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Follow up to RR https://reviews.apache.org/r/35247/, which was not good 
> enough, fixed only one of two problems.
> 
> Using DeclineOffers() instead of Return() should make the master resend 
> offers so we can launch tasks. See line 205 below.
> 
> Following Jie's suggestion, no more CHECK_READY inside launchTask(s)(), but 
> we return a Try instead and follow call sites with EXPECT_SOME(task(s)).
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp 8bd5dd847fb189d0eeeaa760d3ec8ce3af1c2392 
> 
> Diff: https://reviews.apache.org/r/35438/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 35438: Fixed fetcher cache test race for resource offers when starting tasks and changed corresponding CHECK to EXPECT.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [35438]

All tests passed.

- Mesos ReviewBot


On June 14, 2015, 1:54 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35438/
> ---
> 
> (Updated June 14, 2015, 1:54 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Follow up to RR https://reviews.apache.org/r/35247/, which was not good 
> enough, fixed only one of two problems.
> 
> Using DeclineOffers() instead of Return() should make the master resend 
> offers so we can launch tasks. See line 205 below.
> 
> Following Jie's suggestion, no more CHECK_READY inside launchTask(s)(), but 
> we return a Try instead and follow call sites with EXPECT_SOME(task(s)).
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp 8bd5dd847fb189d0eeeaa760d3ec8ce3af1c2392 
> 
> Diff: https://reviews.apache.org/r/35438/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Review Request 35438: Fixed fetcher cache test race for resource offers when starting tasks and changed corresponding CHECK to EXPECT.

[Bernd Mathiske]

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

Review request for mesos, Benjamin Hindman, Timothy Chen, and Vinod Kone.


Repository: mesos


Description
---

Follow up to RR https://reviews.apache.org/r/35247/, which was not good enough, 
fixed only one of two problems.

Using DeclineOffers() instead of Return() should make the master resend offers 
so we can launch tasks. See line 205 below.

Following Jie's suggestion, no more CHECK_READY inside launchTask(s)(), but we 
return a Try instead and follow call sites with EXPECT_SOME(task(s)).


Diffs
-

  src/tests/fetcher_cache_tests.cpp 8bd5dd847fb189d0eeeaa760d3ec8ce3af1c2392 

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


Testing
---

make check


Thanks,

Bernd Mathiske