You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Klaus Ma <kl...@gmail.com> on 2016/01/21 07:24:14 UTC
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/
-----------------------------------------------------------
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"
Thanks,
Klaus Ma
Re: Review Request 42589: Added test case for allocator recover with
Quota.
Posted by Klaus Ma <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42589/#review116725
-----------------------------------------------------------
ping AlexR/Joris, can you help to review this test case for allocator recover with Quota?
- Klaus Ma
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
>
>
Re: Review Request 42589: Added test case for allocator recover with
Quota.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42589/#review115588
-----------------------------------------------------------
Patch looks great!
Reviews applied: [42535, 42589]
Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
- Mesos ReviewBot
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
>
>
Re: Review Request 42589: Added test case for allocator recover with
Quota.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
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 Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42589/#review119138
-----------------------------------------------------------
Patch looks great!
Reviews applied: [42535, 42589]
Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On Feb. 13, 2016, 3:15 a.m., Klaus Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42589/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2016, 3:15 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 990f3723d52dfeaa19d5eb0603c0fc7eb2b362c7
>
> 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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42589/
-----------------------------------------------------------
(Updated Feb. 13, 2016, 11:15 a.m.)
Review request for mesos, Alexander Rukletsov, Ben Mahler, and Joris Van Remoortere.
Changes
-------
rebase and address comments.
Repository: mesos
Description
-------
Added test case for allocator recover with Quota.
Diffs (updated)
-----
src/tests/hierarchical_allocator_tests.cpp 990f3723d52dfeaa19d5eb0603c0fc7eb2b362c7
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>.
-----------------------------------------------------------
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.
Changes
-------
Add description on testing.
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 (updated)
-------
make
make check GTEST_FILTER="HierarchicalAllocatorTest.RecoverWithQuota" (CHECK() failed without fix)
Thanks,
Klaus Ma