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