You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rukletsov <ru...@gmail.com> on 2016/02/01 12:57:17 UTC
Re: Review Request 42589: Added test case for allocator recover with
Quota.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42589/#review117197
-----------------------------------------------------------
What is the use case you want to test here? Specifically, why do you test *both* resume paths simulataneously? Will the outcome of the test change if you remove one of those?
src/tests/hierarchical_allocator_tests.cpp (line 2013)
<https://reviews.apache.org/r/42589/#comment178313>
Why did you drop the comment about pausing the clock?
src/tests/hierarchical_allocator_tests.cpp (line 2019)
<https://reviews.apache.org/r/42589/#comment178317>
Not yours, but we should extract such variables into consts in the fixture's scope to increase readability. I have a review out to fix it: https://reviews.apache.org/r/41950
src/tests/hierarchical_allocator_tests.cpp (lines 2021 - 2023)
<https://reviews.apache.org/r/42589/#comment178319>
I know you're following the pattern here (and I regret introducing it), but I think we should create these instances when we are about to use them.
Btw, I have a pacth out to clean this up: https://reviews.apache.org/r/41950
src/tests/hierarchical_allocator_tests.cpp (line 2023)
<https://reviews.apache.org/r/42589/#comment178318>
Mind leaving a comment why such values? Should quota be exactly 0.25 from availble resources or just smaller?
src/tests/hierarchical_allocator_tests.cpp (line 2025)
<https://reviews.apache.org/r/42589/#comment178320>
Let's stop using "slave" in comments.
src/tests/hierarchical_allocator_tests.cpp (lines 2025 - 2027)
<https://reviews.apache.org/r/42589/#comment178321>
Let's not describe the current recovery algorithm here. When the actual implementation changes, chances are, the comment won't be updated and becomes misleading. Rather, let's describe our *intentions* or *expectations* here.
src/tests/hierarchical_allocator_tests.cpp (line 2041)
<https://reviews.apache.org/r/42589/#comment178322>
Why 11 minutes?
src/tests/hierarchical_allocator_tests.cpp (line 2046)
<https://reviews.apache.org/r/42589/#comment178323>
I know you're following the pattern here, but dont you think it's inconsistent to use a constant for allocations on agents and create a value in-place for framework allocations?
- Alexander Rukletsov
On Jan. 21, 2016, 6:27 a.m., Klaus Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42589/
> -----------------------------------------------------------
>
> (Updated Jan. 21, 2016, 6:27 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Joris Van Remoortere.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added test case for allocator recover with Quota.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 953712149bd951789beb29c72779c4ac65aa48dc
>
> Diff: https://reviews.apache.org/r/42589/diff/
>
>
> Testing
> -------
>
> make
> make check GTEST_FILTER="HierarchicalAllocatorTest.RecoverWithQuota" (CHECK() failed without fix)
>
>
> Thanks,
>
> Klaus Ma
>
>
Re: Review Request 42589: Added test case for allocator recover with
Quota.
Posted by Klaus Ma <kl...@gmail.com>.
> On Feb. 1, 2016, 7:57 p.m., Alexander Rukletsov wrote:
> > What is the use case you want to test here? Specifically, why do you test *both* resume paths simulataneously? Will the outcome of the test change if you remove one of those?
This is the test case for https://reviews.apache.org/r/42535 ; without r42535, this test will crash.
- Klaus
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42589/#review117197
-----------------------------------------------------------
On Jan. 21, 2016, 2:27 p.m., Klaus Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42589/
> -----------------------------------------------------------
>
> (Updated Jan. 21, 2016, 2:27 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Joris Van Remoortere.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added test case for allocator recover with Quota.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 953712149bd951789beb29c72779c4ac65aa48dc
>
> Diff: https://reviews.apache.org/r/42589/diff/
>
>
> Testing
> -------
>
> make
> make check GTEST_FILTER="HierarchicalAllocatorTest.RecoverWithQuota" (CHECK() failed without fix)
>
>
> Thanks,
>
> Klaus Ma
>
>