Re: Review Request 49612: Removed unnecessary `Clock::settle` calls from test cases.

[Neil Conway]

> On July 5, 2016, 3:08 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 3100
> > 
> >
> > I would keep these 3 instances in this test. It's not immediately clear 
> > that `AWAIT` happens in `handleAllocationsAndRecoverResources()`.
> 
> Neil Conway wrote:
> I don't agree with keeping `Clock::settle` in this case. For one thing, 
> it implies the wrong thing: i.e., keeping `Clock::settle` implies that 
> `handleAllocationsAndRecoverResources()` does not do sufficient 
> synchronization on its own, which is incorrect.
> 
> Second, `Clock::settle` is a kludge; it should only be used when there is 
> _no other way_ to achieve the synchronization required by the test. So I'd 
> prefer to discourage its use whenever possible.
> 
> Alexander Rukletsov wrote:
> In general, `Clock::settle` is not a kludge: it allows to make sure a 
> certain event is processed before another event is scheduled.
> 
> It is not the case here, though. We usually write tests in a way that 
> people can easily follow main steps without looking into functions (hence a 
> lot od copy paste code instead of fixture methods). 
> `handleAllocationsAndRecoverResources()` already breaks this convention by 
> modifying the caller's context; removing `Clock::settle` makes things even 
> worse IMO.
> 
> Neil Conway wrote:
> `Clock::settle` is a kludge because it hides the synchronization 
> dependencies between operations. If a test needs to wait for operation X to 
> complete before it does operation X, then X should provide an explicit 
> interface (e.g., return a `Future` or using GMock trickery) to allow the test 
> code to wait for X to complete. `Clock::settle` hides the synchronization 
> dependency between X and Y. Moreover, `Clock::settle` waits for _all_ 
> outstanding messages to be consumed, which is overkill. It is also very easy 
> to write subtly incorrect code using `Clock::settle` -- e.g., look at all the 
> stuff that breaks if you remove the `os::sleep()` call from the 
> `Clock::settle` implementation.
> 
> I agree that `handleAllocationsAndRecoverResources()` is somewhat 
> inconsistent with our usual convention for test cases, but I don't think that 
> the presence/absence of `Clock::settle` makes things any better/worse.
> 
> Alexander Rukletsov wrote:
> Yeah, from this point of view it is a kludge : ). Until we have 
> primitives, that allow us to express operation dependenices in tests, we will 
> have to use `Clock::settle()` and hence educate contributors how to use it 
> properly, especially the difference between performing an action and a 
> corresponding event being put into the actor's mailbox.
> 
> My concern is that with both `Clock::settle()` and `AWAIT_*` absent the 
> dependency between operations is hidden. 
> `handleAllocationsAndRecoverResources()` does not hint that it awaits for 
> allocations to be made. Maybe making it a local lambda and renaming to 
> something like `awaitAllocationsAndRecoverResources()` will help?

Per discussion, renamed the function and made it a lambda: 
https://reviews.apache.org/r/49883/


- Neil


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


On July 5, 2016, 8:51 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49612/
> ---
> 
> (Updated July 5, 2016, 8:51 a.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a test case calls `Clock::settle` and then immediately waits for a
> future to be completed, settling the clock is usually unnecessary.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 0498cd5e54b0e4b87a767585a77699653aa52179 
> 
> Diff: https://reviews.apache.org/r/49612/diff/
> 
> 
> Testing
> ---
> 
> `mesos-tests --gtest_filter=HierarchicalAllocatorTest.* --gtest_repeat=500 
> --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 49612: Removed unnecessary `Clock::settle` calls from test cases.

[Alexander Rukletsov]

> On July 5, 2016, 3:08 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 3100
> > 
> >
> > I would keep these 3 instances in this test. It's not immediately clear 
> > that `AWAIT` happens in `handleAllocationsAndRecoverResources()`.
> 
> Neil Conway wrote:
> I don't agree with keeping `Clock::settle` in this case. For one thing, 
> it implies the wrong thing: i.e., keeping `Clock::settle` implies that 
> `handleAllocationsAndRecoverResources()` does not do sufficient 
> synchronization on its own, which is incorrect.
> 
> Second, `Clock::settle` is a kludge; it should only be used when there is 
> _no other way_ to achieve the synchronization required by the test. So I'd 
> prefer to discourage its use whenever possible.
> 
> Alexander Rukletsov wrote:
> In general, `Clock::settle` is not a kludge: it allows to make sure a 
> certain event is processed before another event is scheduled.
> 
> It is not the case here, though. We usually write tests in a way that 
> people can easily follow main steps without looking into functions (hence a 
> lot od copy paste code instead of fixture methods). 
> `handleAllocationsAndRecoverResources()` already breaks this convention by 
> modifying the caller's context; removing `Clock::settle` makes things even 
> worse IMO.
> 
> Neil Conway wrote:
> `Clock::settle` is a kludge because it hides the synchronization 
> dependencies between operations. If a test needs to wait for operation X to 
> complete before it does operation X, then X should provide an explicit 
> interface (e.g., return a `Future` or using GMock trickery) to allow the test 
> code to wait for X to complete. `Clock::settle` hides the synchronization 
> dependency between X and Y. Moreover, `Clock::settle` waits for _all_ 
> outstanding messages to be consumed, which is overkill. It is also very easy 
> to write subtly incorrect code using `Clock::settle` -- e.g., look at all the 
> stuff that breaks if you remove the `os::sleep()` call from the 
> `Clock::settle` implementation.
> 
> I agree that `handleAllocationsAndRecoverResources()` is somewhat 
> inconsistent with our usual convention for test cases, but I don't think that 
> the presence/absence of `Clock::settle` makes things any better/worse.

Yeah, from this point of view it is a kludge : ). Until we have primitives, 
that allow us to express operation dependenices in tests, we will have to use 
`Clock::settle()` and hence educate contributors how to use it properly, 
especially the difference between performing an action and a corresponding 
event being put into the actor's mailbox.

My concern is that with both `Clock::settle()` and `AWAIT_*` absent the 
dependency between operations is hidden. 
`handleAllocationsAndRecoverResources()` does not hint that it awaits for 
allocations to be made. Maybe making it a local lambda and renaming to 
something like `awaitAllocationsAndRecoverResources()` will help?


- Alexander


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


On July 5, 2016, 8:51 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49612/
> ---
> 
> (Updated July 5, 2016, 8:51 a.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a test case calls `Clock::settle` and then immediately waits for a
> future to be completed, settling the clock is usually unnecessary.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 0498cd5e54b0e4b87a767585a77699653aa52179 
> 
> Diff: https://reviews.apache.org/r/49612/diff/
> 
> 
> Testing
> ---
> 
> `mesos-tests --gtest_filter=HierarchicalAllocatorTest.* --gtest_repeat=500 
> --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 49612: Removed unnecessary `Clock::settle` calls from test cases.

[Neil Conway]

> On July 5, 2016, 3:08 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 3100
> > 
> >
> > I would keep these 3 instances in this test. It's not immediately clear 
> > that `AWAIT` happens in `handleAllocationsAndRecoverResources()`.
> 
> Neil Conway wrote:
> I don't agree with keeping `Clock::settle` in this case. For one thing, 
> it implies the wrong thing: i.e., keeping `Clock::settle` implies that 
> `handleAllocationsAndRecoverResources()` does not do sufficient 
> synchronization on its own, which is incorrect.
> 
> Second, `Clock::settle` is a kludge; it should only be used when there is 
> _no other way_ to achieve the synchronization required by the test. So I'd 
> prefer to discourage its use whenever possible.
> 
> Alexander Rukletsov wrote:
> In general, `Clock::settle` is not a kludge: it allows to make sure a 
> certain event is processed before another event is scheduled.
> 
> It is not the case here, though. We usually write tests in a way that 
> people can easily follow main steps without looking into functions (hence a 
> lot od copy paste code instead of fixture methods). 
> `handleAllocationsAndRecoverResources()` already breaks this convention by 
> modifying the caller's context; removing `Clock::settle` makes things even 
> worse IMO.

`Clock::settle` is a kludge because it hides the synchronization dependencies 
between operations. If a test needs to wait for operation X to complete before 
it does operation X, then X should provide an explicit interface (e.g., return 
a `Future` or using GMock trickery) to allow the test code to wait for X to 
complete. `Clock::settle` hides the synchronization dependency between X and Y. 
Moreover, `Clock::settle` waits for _all_ outstanding messages to be consumed, 
which is overkill. It is also very easy to write subtly incorrect code using 
`Clock::settle` -- e.g., look at all the stuff that breaks if you remove the 
`os::sleep()` call from the `Clock::settle` implementation.

I agree that `handleAllocationsAndRecoverResources()` is somewhat inconsistent 
with our usual convention for test cases, but I don't think that the 
presence/absence of `Clock::settle` makes things any better/worse.


- Neil


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


On July 5, 2016, 8:51 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49612/
> ---
> 
> (Updated July 5, 2016, 8:51 a.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a test case calls `Clock::settle` and then immediately waits for a
> future to be completed, settling the clock is usually unnecessary.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 0498cd5e54b0e4b87a767585a77699653aa52179 
> 
> Diff: https://reviews.apache.org/r/49612/diff/
> 
> 
> Testing
> ---
> 
> `mesos-tests --gtest_filter=HierarchicalAllocatorTest.* --gtest_repeat=500 
> --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 49612: Removed unnecessary `Clock::settle` calls from test cases.

[Alexander Rukletsov]

> On July 5, 2016, 3:08 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 3100
> > 
> >
> > I would keep these 3 instances in this test. It's not immediately clear 
> > that `AWAIT` happens in `handleAllocationsAndRecoverResources()`.
> 
> Neil Conway wrote:
> I don't agree with keeping `Clock::settle` in this case. For one thing, 
> it implies the wrong thing: i.e., keeping `Clock::settle` implies that 
> `handleAllocationsAndRecoverResources()` does not do sufficient 
> synchronization on its own, which is incorrect.
> 
> Second, `Clock::settle` is a kludge; it should only be used when there is 
> _no other way_ to achieve the synchronization required by the test. So I'd 
> prefer to discourage its use whenever possible.

In general, `Clock::settle` is not a kludge: it allows to make sure a certain 
event is processed before another event is scheduled.

It is not the case here, though. We usually write tests in a way that people 
can easily follow main steps without looking into functions (hence a lot od 
copy paste code instead of fixture methods). 
`handleAllocationsAndRecoverResources()` already breaks this convention by 
modifying the caller's context; removing `Clock::settle` makes things even 
worse IMO.


- Alexander


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


On July 5, 2016, 8:51 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49612/
> ---
> 
> (Updated July 5, 2016, 8:51 a.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a test case calls `Clock::settle` and then immediately waits for a
> future to be completed, settling the clock is usually unnecessary.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 0498cd5e54b0e4b87a767585a77699653aa52179 
> 
> Diff: https://reviews.apache.org/r/49612/diff/
> 
> 
> Testing
> ---
> 
> `mesos-tests --gtest_filter=HierarchicalAllocatorTest.* --gtest_repeat=500 
> --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 49612: Removed unnecessary `Clock::settle` calls from test cases.

[Neil Conway]

> On July 5, 2016, 3:08 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 3100
> > 
> >
> > I would keep these 3 instances in this test. It's not immediately clear 
> > that `AWAIT` happens in `handleAllocationsAndRecoverResources()`.

I don't agree with keeping `Clock::settle` in this case. For one thing, it 
implies the wrong thing: i.e., keeping `Clock::settle` implies that 
`handleAllocationsAndRecoverResources()` does not do sufficient synchronization 
on its own, which is incorrect.

Second, `Clock::settle` is a kludge; it should only be used when there is _no 
other way_ to achieve the synchronization required by the test. So I'd prefer 
to discourage its use whenever possible.


- Neil


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


On July 5, 2016, 8:51 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49612/
> ---
> 
> (Updated July 5, 2016, 8:51 a.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a test case calls `Clock::settle` and then immediately waits for a
> future to be completed, settling the clock is usually unnecessary.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 0498cd5e54b0e4b87a767585a77699653aa52179 
> 
> Diff: https://reviews.apache.org/r/49612/diff/
> 
> 
> Testing
> ---
> 
> `mesos-tests --gtest_filter=HierarchicalAllocatorTest.* --gtest_repeat=500 
> --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 49612: Removed unnecessary `Clock::settle` calls from test cases.

[Alexander Rukletsov]

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


Fix it, then Ship it!





src/tests/hierarchical_allocator_tests.cpp 


I would keep these 3 instances in this test. It's not immediately clear 
that `AWAIT` happens in `handleAllocationsAndRecoverResources()`.


- Alexander Rukletsov


On July 5, 2016, 8:51 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49612/
> ---
> 
> (Updated July 5, 2016, 8:51 a.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a test case calls `Clock::settle` and then immediately waits for a
> future to be completed, settling the clock is usually unnecessary.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 0498cd5e54b0e4b87a767585a77699653aa52179 
> 
> Diff: https://reviews.apache.org/r/49612/diff/
> 
> 
> Testing
> ---
> 
> `mesos-tests --gtest_filter=HierarchicalAllocatorTest.* --gtest_repeat=500 
> --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 49612: Removed unnecessary `Clock::settle` calls from test cases.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49610, 49612]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On July 5, 2016, 8:51 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49612/
> ---
> 
> (Updated July 5, 2016, 8:51 a.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a test case calls `Clock::settle` and then immediately waits for a
> future to be completed, settling the clock is usually unnecessary.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 0498cd5e54b0e4b87a767585a77699653aa52179 
> 
> Diff: https://reviews.apache.org/r/49612/diff/
> 
> 
> Testing
> ---
> 
> `mesos-tests --gtest_filter=HierarchicalAllocatorTest.* --gtest_repeat=500 
> --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 49612: Removed unnecessary `Clock::settle` calls from test cases.

[Neil Conway]

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

(Updated July 5, 2016, 8:51 a.m.)


Review request for mesos and Alexander Rukletsov.


Changes
---

Rebase.


Repository: mesos


Description
---

If a test case calls `Clock::settle` and then immediately waits for a
future to be completed, settling the clock is usually unnecessary.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
0498cd5e54b0e4b87a767585a77699653aa52179 

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


Testing
---

`mesos-tests --gtest_filter=HierarchicalAllocatorTest.* --gtest_repeat=500 
--gtest_break_on_failure`


Thanks,

Neil Conway

Re: Review Request 49612: Removed unnecessary `Clock::settle` calls from test cases.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49610, 49612]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On July 4, 2016, 8:40 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49612/
> ---
> 
> (Updated July 4, 2016, 8:40 p.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a test case calls `Clock::settle` and then immediately waits for a
> future to be completed, settling the clock is usually unnecessary.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 0498cd5e54b0e4b87a767585a77699653aa52179 
> 
> Diff: https://reviews.apache.org/r/49612/diff/
> 
> 
> Testing
> ---
> 
> `mesos-tests --gtest_filter=HierarchicalAllocatorTest.* --gtest_repeat=500 
> --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>