mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Rukletsov <ruklet...@gmail.com>
Subject Re: Review Request 42589: Added test case for allocator recover with Quota.
Date Mon, 01 Feb 2016 11:57:17 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message