----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41672/#review119839 -----------------------------------------------------------
src/tests/hierarchical_allocator_tests.cpp (line 2307) <https://reviews.apache.org/r/41672/#comment181212> Let's abolish slavery in this test : ). Here and below in comments and variable namings. src/tests/hierarchical_allocator_tests.cpp (lines 2320 - 2324) <https://reviews.apache.org/r/41672/#comment181213> This is not related to "register agents with the same resources" section, right? Let's move it up and write why this is necessary. src/tests/hierarchical_allocator_tests.cpp (line 2325) <https://reviews.apache.org/r/41672/#comment181223> Why post-increment? Is it consistent to the codebase? src/tests/hierarchical_allocator_tests.cpp (line 2329) <https://reviews.apache.org/r/41672/#comment181215> Why not merging this loop with the previous one? src/tests/hierarchical_allocator_tests.cpp (line 2335) <https://reviews.apache.org/r/41672/#comment181214> `{}` will do the job. src/tests/hierarchical_allocator_tests.cpp (line 2337) <https://reviews.apache.org/r/41672/#comment181217> Here is a good place to insert a comment describing the cluster state. Look for an example in other tests. src/tests/hierarchical_allocator_tests.cpp (line 2338) <https://reviews.apache.org/r/41672/#comment181219> Backticks instead of single quotes, please! Here and everywhere. src/tests/hierarchical_allocator_tests.cpp (line 2342) <https://reviews.apache.org/r/41672/#comment181220> `{}` will do the job. Here and below. src/tests/hierarchical_allocator_tests.cpp (lines 2350 - 2354) <https://reviews.apache.org/r/41672/#comment181221> How about separating this in two sections: check allocation state and recovering resources? Between these sections you can insert a comment describing the cluster state (I've mentioned that above already). src/tests/hierarchical_allocator_tests.cpp (lines 2371 - 2397) <https://reviews.apache.org/r/41672/#comment181225> You can wrap this block in curly braces and avoid adding numerals to variable names. Same for the block below. src/tests/hierarchical_allocator_tests.cpp (line 2373) <https://reviews.apache.org/r/41672/#comment181229> You can put framework id's into a `std::set` instead. Given there are two allocations, there is a guarantee, that there will be no attempts to put the same framework into the set twice. src/tests/hierarchical_allocator_tests.cpp (line 2374) <https://reviews.apache.org/r/41672/#comment181224> Let's pull advancing clock above so that it's not lost between lines. src/tests/hierarchical_allocator_tests.cpp (lines 2402 - 2403) <https://reviews.apache.org/r/41672/#comment181230> If you use `set` as proposed above, you can check set against set here. src/tests/hierarchical_allocator_tests.cpp (lines 2413 - 2414) <https://reviews.apache.org/r/41672/#comment181226> You can avoid this by wrapping this section in curly braces. src/tests/hierarchical_allocator_tests.cpp (lines 2423 - 2433) <https://reviews.apache.org/r/41672/#comment181231> Will it be cleaner to put resources from the allocation into a hashmap by framework id? I think this way you can even get rid of the loop. Same suggestions for the section below. - Alexander Rukletsov On Jan. 20, 2016, 8:36 a.m., Yongqiao Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41672/ > ----------------------------------------------------------- > > (Updated Jan. 20, 2016, 8:36 a.m.) > > > Review request for mesos, Adam B, Neil Conway, and Qian Zhang. > > > Bugs: MESOS-4200 > https://issues.apache.org/jira/browse/MESOS-4200 > > > Repository: mesos > > > Description > ------- > > Test case(s) for weights + allocation behaviour. > > > Diffs > ----- > > src/tests/hierarchical_allocator_tests.cpp > 953712149bd951789beb29c72779c4ac65aa48dc > > Diff: https://reviews.apache.org/r/41672/diff/ > > > Testing > ------- > > Make check done: > $ ./src/mesos-tests --gtest_filter=HierarchicalAllocatorTest.UpdateWeight > Source directory: /Users/yqwyq/Desktop/mesos > Build directory: /Users/yqwyq/Desktop/mesos/build > [==========] Running 1 test from 1 test case. > [----------] Global test environment set-up. > [----------] 1 test from HierarchicalAllocatorTest > [ RUN ] HierarchicalAllocatorTest.UpdateWeight > [ OK ] HierarchicalAllocatorTest.UpdateWeight (87 ms) > [----------] 1 test from HierarchicalAllocatorTest (87 ms total) > > [----------] Global test environment tear-down > [==========] 1 test from 1 test case ran. (176 ms total) > [ PASSED ] 1 test. > > > Thanks, > > Yongqiao Wang > >
