----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49616/#review143308 -----------------------------------------------------------
Thanks Jacob! I left some comments here around style and readability of the code. It looks like much of this was copied and so I realize that you weren't the one to introduce many of these patterns. Nonetheless, would be great to simplify this! src/tests/hierarchical_allocator_tests.cpp (lines 3574 - 3575) <https://reviews.apache.org/r/49616/#comment209073> We use size_t as the type of "size" / "count" / "index" variables in general, as it provides some additional information to the reader: ``` $ grep 'for (size_t i' src -R | wc -l 66 $ grep 'for (unsigned i' src -R | wc -l 14 ``` src/tests/hierarchical_allocator_tests.cpp (lines 3579 - 3581) <https://reviews.apache.org/r/49616/#comment209072> I'm confused by this comment, why is this relevant if the clock is paused? src/tests/hierarchical_allocator_tests.cpp (line 3586) <https://reviews.apache.org/r/49616/#comment209074> For structs, classes, methods, the braces go on the next line. Unfortunately we don't have a style checker for this yet! How about s/OfferedResources/Allocation/ ? This would match the pattern used in the HierarchicalAllocatorTestBase. It's a bit unfortunate that we can't use the existing pattern of pushing into a process::Queue. src/tests/hierarchical_allocator_tests.cpp (lines 3598 - 3601) <https://reviews.apache.org/r/49616/#comment209075> Seems this can be made a bit clearer: ``` foreachpair (const SlaveID& slaveId, const Resources& r, resources) { Offer offer; offer.frameworkId = frameworkId; offer.slaveId = slaveId; offer.resources = r; offers.push_back(std::move(offer)); } ``` Note that we don't make much use of the brace initilization of structs yet, as it tends to be a bit less readable than explicitly setting each member here. src/tests/hierarchical_allocator_tests.cpp (line 3607) <https://reviews.apache.org/r/49616/#comment209076> We tend to declare variables close to where they are needed, seems this can be moved down to where we create the slaves below? src/tests/hierarchical_allocator_tests.cpp (lines 3620 - 3621) <https://reviews.apache.org/r/49616/#comment209077> Can we name this something a bit more helpful to the reader? My initial intuition was that this would represent the agent's resources, but this seems to be the allocation? How about s/resources/allocation/ ? Seems this can be made clearer by pulling the agent's resources up? ``` // Each agent has a portion of it's resources allocated to a single // framework. We round-robin through the frameworks when allocating. Resources agentTotal = Resources::parse( "cpus:24;mem:4096;disk:4096;ports:[31000-32000]").get(); Resources allocation = Resources::parse( "cpus:16;mem:1024;disk:1024").get(); allocation += createPorts(fragment(createRange(31000, 32000), 16)); for (...) { ... } ``` src/tests/hierarchical_allocator_tests.cpp (lines 3630 - 3631) <https://reviews.apache.org/r/49616/#comment209078> This comment seems inaccurate? There seems to be more than 1 port allocated. Also, no need to mention tasks here (because the allocator has no knowlege of them, it also has no knowledge of executors). We can just have the allocation comment above the loop (per my other suggestion)? src/tests/hierarchical_allocator_tests.cpp (lines 3641 - 3644) <https://reviews.apache.org/r/49616/#comment209079> It took me some time to understand this, how about the following comment to help the reader: ``` // Now perform allocations. Each time we trigger an allocation run, we // increase the number of frameworks that are suppressing offers. To // ensure the test can run in a timely manner, we always perform a // fixed number of allocations. ``` It seems to me to be more helpful to call have a '`size_t allocations = 5`' rather than naming this '`batches`'. With this I would have been able to understand more quickly. Note that we try to use a newline between the end of a comment and the start of a TODO block: ``` // Loop through the frameworks in batches, suppressing // as we go and measuring allocation times. // // TODO(jjanco): Parameterize the test by batch size, not an arbitrary number. // Batching reduces loop size, lowering time to test completion. ``` The idea is to treat these like separate comment paragraphs and to improve the readability. Without the newlines, it's a bit tricker to tell if your last sentence belongs to the TODO or the comment. src/tests/hierarchical_allocator_tests.cpp (lines 3647 - 3648) <https://reviews.apache.org/r/49616/#comment209080> This comment and math is a bit confusing, where is a ceiling being used? Do we need the example? If we do, can we make it a bit simpler than 103 and 21? Even just using small numbers here would be easier on the reader. src/tests/hierarchical_allocator_tests.cpp (lines 3649 - 3652) <https://reviews.apache.org/r/49616/#comment209081> Per my comment above, it seems much easier to understand this code if we just loop on the number of allocations we expect: ``` size_t allocationsCount = 5; size_t suppressCount = 0; for (size_t i = 0; i < allocationsCount; ++i) { ... // Suppress another batch of frameworks. for (size_t j = 0; j < frameworkCount / allocationsCount; ++j) { allocator->suppressOffers(frameworks[suppressCount].id()); ++suppressCount; } ... } ``` src/tests/hierarchical_allocator_tests.cpp (line 3657) <https://reviews.apache.org/r/49616/#comment209082> Can you avoid auto here? Would be more readable to have the type, and we only use 'auto' when the type is very verbose or is clear using only local reasonsing (here I need to look up towards the top of the test to reason about the type of 'offer', we call this "non-local" reasoning since it is somewhat distant from 'offer' here). - Benjamin Mahler On July 8, 2016, 5:55 p.m., Jacob Janco wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49616/ > ----------------------------------------------------------- > > (Updated July 8, 2016, 5:55 p.m.) > > > Review request for mesos, James Peach, Joris Van Remoortere, and Jiang Yan Xu. > > > Bugs: MESOS-5781 > https://issues.apache.org/jira/browse/MESOS-5781 > > > Repository: mesos > > > Description > ------- > > - Useful for high framework clusters which carry > many suppressed frameworks that are considered > during allocation. > > > Diffs > ----- > > src/tests/hierarchical_allocator_tests.cpp > 0498cd5e54b0e4b87a767585a77699653aa52179 > > Diff: https://reviews.apache.org/r/49616/diff/ > > > Testing > ------- > > MESOS_BENCHMARK=1 GTEST_FILTER="*BENCHMARK_Test.SuppressOffers*" make check > > > Thanks, > > Jacob Janco > >
