mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 49616: Add suppression benchmark.
Date Sat, 23 Jul 2016 01:37:08 GMT

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


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