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

Reply via email to