mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ben Mahler <benjamin.mah...@gmail.com>
Subject Re: Review Request 43883: Added a metric for querying the number offer filters for a role.
Date Sat, 26 Mar 2016 00:12:26 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43883/#review125485
-----------------------------------------------------------



The code looks pretty clean, thanks! However, there were two bugs here:

(1) The value of the metric only counts how many slaves have offer filters, rather than the
total number of filters (note the value type of `offerFilters` is a set).
(2) removeRole was not removing the role from the map of gauges

I made some minor adjustments based on the comments here and fixed these two bugs, will commit
shortly!


docs/monitoring.md (line 955)
<https://reviews.apache.org/r/43883/#comment188276>

    In the interest of consistency and avoiding repeating ourselves in the name, perhaps the
following?
    
    ```
    allocator/mesos/filters/roles/<role>/active
    ```



src/master/allocator/mesos/hierarchical.hpp (line 298)
<https://reviews.apache.org/r/43883/#comment188273>

    Hm.. why the "role" naming prefix here but no "role" naming prefix on `_quota_allocated`?



src/master/allocator/mesos/hierarchical.cpp (line 1742)
<https://reviews.apache.org/r/43883/#comment188275>

    2 spaces here



src/master/allocator/mesos/hierarchical.cpp (line 1743)
<https://reviews.apache.org/r/43883/#comment188281>

    How about wrapping the argument on the next line here for consistency?



src/master/allocator/mesos/hierarchical.cpp (line 1747)
<https://reviews.apache.org/r/43883/#comment188277>

    whoops, extra newline?



src/master/allocator/mesos/hierarchical.cpp (line 1753)
<https://reviews.apache.org/r/43883/#comment188278>

    Whoops, this looks like a bug. `offerFilters` is a `hashmap<SlaveID, hashset<OfferFilter*>>`
and so it looks like we need to loop over the values and use the sum of the sizes, since there
may be more than one filter per slave:
    
    ```
    foreachkey (const SlaveID& slaveId, framework.offerFilters) {
      result += framework.offerFilters[slaveId].size();
    }
    ```



src/master/allocator/mesos/metrics.hpp (lines 51 - 52)
<https://reviews.apache.org/r/43883/#comment188274>

    Very nice!



src/master/allocator/mesos/metrics.cpp (line 113)
<https://reviews.apache.org/r/43883/#comment188272>

    s/(/ (/



src/master/allocator/mesos/metrics.cpp (line 175)
<https://reviews.apache.org/r/43883/#comment188279>

    `offer_filter` here but `gauge` in removeRole?



src/master/allocator/mesos/metrics.cpp (line 185)
<https://reviews.apache.org/r/43883/#comment188271>

    2 spaces here



src/master/allocator/mesos/metrics.cpp (lines 188 - 192)
<https://reviews.apache.org/r/43883/#comment188285>

    This looks like a bug: we also have to remove the role from the map here.



src/tests/hierarchical_allocator_tests.cpp (line 568)
<https://reviews.apache.org/r/43883/#comment188280>

    Since the metric is declared higher up it's a bit tricker to follow what is being checked
here, how about s/metric/filters_active/ ?



src/tests/hierarchical_allocator_tests.cpp (lines 2655 - 2657)
<https://reviews.apache.org/r/43883/#comment188283>

    Thanks for the test!



src/tests/hierarchical_allocator_tests.cpp (lines 2700 - 2707)
<https://reviews.apache.org/r/43883/#comment188282>

    Some newlines here would be nice:
    
    ```
      Future<Allocation> allocation = allocations.get();
    
      AWAIT_READY(allocation);
      ASSERT_EQ(framework1.id(), allocation->frameworkId);
    
      allocator->recoverResources(
          allocation->frameworkId,
          agent.id(),
          allocation->resources.get(agent.id()).get(),
          offerFilter);
    ```


- Ben Mahler


On March 24, 2016, 1:36 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43883/
> -----------------------------------------------------------
> 
> (Updated March 24, 2016, 1:36 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4722
>     https://issues.apache.org/jira/browse/MESOS-4722
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added a metric for querying the number offer filters for a role.
> 
> 
> Diffs
> -----
> 
>   docs/monitoring.md dcf19e6ce06b02373a2bd1af81a451a35743fa76 
>   src/master/allocator/mesos/hierarchical.hpp e4604f4da59166da24709a68b8cd4e56bf55f97f

>   src/master/allocator/mesos/hierarchical.cpp 39a290d0db2c22e179a8f933b1a78e3a2dcefdc3

>   src/master/allocator/mesos/metrics.hpp b5f2806cff99ee2ee46c4ac8a13174ef699969aa 
>   src/master/allocator/mesos/metrics.cpp 2f3012a5bd40a4100e790e1d373832015c80b993 
>   src/tests/hierarchical_allocator_tests.cpp e9cfcfc0ad8b0b89bbac459b7db39183f6c189be

> 
> Diff: https://reviews.apache.org/r/43883/diff/
> 
> 
> Testing
> -------
> 
> make check (OS X)
> 
> I confirmed that this does not lead to general performance regressions in the allocator;
this is partially expected since the added code only inserts metrics in the allocator while
the actual work is perform asynchronously. These tests where performed with `HierarchicalAllocator_BENCHMARK_Test.DeclineOffers`
on an optimized build under OS X using clang(trunk) as compiler.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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