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 43884: Added allocator metrics for used quotas.
Date Thu, 17 Mar 2016 21:07:00 GMT

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



Thanks Benjamin! The main higher level suggestion here is to add the guarantees alongside
the allocations.


docs/monitoring.md (lines 876 - 883)
<https://reviews.apache.org/r/43884/#comment186445>

    Thanks! How about:
    
    ```
    <tr>
      <td>
      <code>allocator/mesos/quota/<role>/<resource>_allocated</code>
      </td>
      <td>Amount of resources considered allocated towards a role's quota guarantee</td>
      <td>Gauge</td>
    </tr>
    ```
    
    Also, can you add the guarantee to this patch?
    
    ```
    <tr>
      <td>
      <code>allocator/mesos/quota/<role>/<resource>_guarantee</code>
      </td>
      <td>Amount of resources guaranteed for a role via quota</td>
      <td>Gauge</td>
    </tr>
    ```
    
    Then we can easily monitor and alert on the distance between the two, and you can imagine
future additions to enable better visibility into the system, for example:
    
    ```
    allocator/mesos/quota/role/cpus_guarantee
    allocator/mesos/quota/role/cpus_allocated
    allocator/mesos/quota/role/cpus_filtered
    allocator/mesos/quota/role/cpus_unsatisfiable
    ```



src/master/allocator/mesos/hierarchical.hpp (lines 290 - 291)
<https://reviews.apache.org/r/43884/#comment186436>

    Could you wrap the arguments each on their own line to be more consistent with our typical
signature wrapping?



src/master/allocator/mesos/hierarchical.cpp (lines 1691 - 1692)
<https://reviews.apache.org/r/43884/#comment186431>

    How about s/resourceName/resource/ here? Looking at the master metrics code that seems
to be more consistent, and this looks to be the first time we've used it:
    
    ```
    $ grep -R resourceName src
    $
    ```



src/master/allocator/mesos/hierarchical.cpp (line 1692)
<https://reviews.apache.org/r/43884/#comment186432>

    Can you wrap each argument on its own line? That would be more consistent with the wrapping
we usually do for function signatures.



src/master/allocator/mesos/hierarchical.cpp (lines 1694 - 1696)
<https://reviews.apache.org/r/43884/#comment186433>

    How about the folllowing wrapping:
    
    ```
      Option<Value::Scalar> used =
        quotaRoleSorter->allocationScalarQuantities(role)
          .get<Value::Scalar>(resource);
    ```



src/master/allocator/mesos/hierarchical.cpp (line 1698)
<https://reviews.apache.org/r/43884/#comment186430>

    Recall that we have an `->` operator now to avoid the `.get().` pattern:
    
    ```
    return used.isSome() ? used->value() : 0;
    ```



src/master/allocator/mesos/metrics.hpp (lines 35 - 38)
<https://reviews.apache.org/r/43884/#comment186429>

    I don't know, this class is so tightly coupled to the allocator process that I'd rather
not have the NOTE (I see this as an embedded struct in the allocator that we've just happened
to have pulled up). How about just the following:
    
    ```
    // Collection of metrics for the allocator, these begin with
    // the following prefix: "allocator/mesos/".
    ```



src/master/allocator/mesos/metrics.hpp (lines 41 - 42)
<https://reviews.apache.org/r/43884/#comment186427>

    Actually, could we make `Metrics` a struct and remove the access qualifiers? This is the
pattern in place for the master's metrics, these Metrics containers are generally meant to
just be a wrapper around the metrics rather than a full fledged abstraction, so we made it
a struct and didn't bother with access qualifiers.
    
    Generally member variables go below member functions, could you move this down?
    
    Also, any reason you needed the pointer instead of a PID? It looks like we could just
defer using the PID here?



src/master/allocator/mesos/metrics.hpp (lines 47 - 50)
<https://reviews.apache.org/r/43884/#comment186428>

    I don't follow why the comment here means that we had to remove the qualifiers.. do you
need this? If not, let's take them out for now since we should just think about this as a
'struct' rather than a 'class'.



src/master/allocator/mesos/metrics.cpp (lines 59 - 64)
<https://reviews.apache.org/r/43884/#comment186412>

    How about avoiding the need for the klunky typedef here:
    
    ```
    foreachkey (const string& role, quota_allocated) {
      foreachvalue (const Gauge& gauge, quota_allocated[role]) {
        metrics::remove(gauge);
      }
    }
    ```



src/master/allocator/mesos/metrics.cpp (lines 68 - 87)
<https://reviews.apache.org/r/43884/#comment186407>

    Could we encode our assumption that this is not called to change quota, but rather only
called to initially specify a quota? In other words, adding a CHECK here that we don't already
know about this role in terms of quota. Otherwise, it will silently misbehave as we discussed
(leak metrics).



src/master/allocator/mesos/metrics.cpp (line 70)
<https://reviews.apache.org/r/43884/#comment186409>

    How about s/quotaedResources/gauges/ here? I also find the use of "quotaed" unfortunate
(I'm guessing you borrowed this from the quota code) since there isn't a clear difference
between `quotaResources` and `quotaedResources` to me.



src/master/allocator/mesos/metrics.cpp (line 73)
<https://reviews.apache.org/r/43884/#comment186408>

    this is actually a copy since the right hand side is not a temporary, it looks like you
only added this to make the gauge name fit in 80 characters?
    
    I would prefer `resource.name()` to having the extra variable with the same name to save
3 characters, or just s/resourceName/name/. If you create the gauge as a variable you can
avoid the need for the variable, and it tends to read easier:
    
    ```
      Gauge gauge = Gauge(
          "allocator/mesos/quota/" + role + "/" + resource.name() + "_allocated",
          defer(...)
      );
      
      metrics::add(gauge);
      
      gauges[resource.name()] = gauge;
    ```



src/master/allocator/mesos/metrics.cpp (lines 79 - 81)
<https://reviews.apache.org/r/43884/#comment186426>

    Why the defer indirection here as opposed to direct defers? Per our offline conversation
this was because we made the member functions const? If so, let's make them non-const for
now as that's the pattern we've put in place to deal with the defer to const issue.
    
    Would love to see the defer to const issue fixed, but let's tackle that later :)



src/master/allocator/mesos/metrics.cpp (line 86)
<https://reviews.apache.org/r/43884/#comment186410>

    How about using the [] operator? It tends to make the key and value read more easily:
    
    ```
    quota_allocated[role] = gauges;
    ```



src/master/allocator/mesos/metrics.cpp (lines 92 - 99)
<https://reviews.apache.org/r/43884/#comment186411>

    How about a CHECK on .contains, then using the [] in the foreachvalue loop? This avoids
the need for the extra `roleQuotaGauges` variable:
    
    ```
    CHECK(quota_allocated.contains(role));
    
    foreachvalue (const Gauge& gauge, quota_allocated[role]) {
      process::metrics::remove(gauge);
    }
    
    quota_allocated.erase(role);
    ```



src/tests/hierarchical_allocator_tests.cpp (lines 1660 - 1664)
<https://reviews.apache.org/r/43884/#comment186406>

    s/metricKey/metric/ would be more consistent
    
    Is it possible to use the JSON contains helper here to make the test a bit easier to read?
Any reason you avoided checking the values of these?
    
    ```
    JSON::Object expected = {
      {"allocator/mesos/quota/" + QUOTA_ROLE + "/cpus_allocated", x},
      {"allocator/mesos/quota/" + QUOTA_ROLE + "/mem_allocated", y},
    };
    
    EXPECT_TRUE(metrics.contains(expected));
    ```
    
    Ditto below


- Ben Mahler


On March 15, 2016, 2:51 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43884/
> -----------------------------------------------------------
> 
> (Updated March 15, 2016, 2:51 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4723
>     https://issues.apache.org/jira/browse/MESOS-4723
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added allocator metrics for used quotas.
> 
> 
> Diffs
> -----
> 
>   docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
>   src/master/allocator/mesos/hierarchical.hpp 52b3a9bfbe73d24968f7ddb0672aee1e05636ab0

>   src/master/allocator/mesos/hierarchical.cpp 70291075c00a9a557529c2562dedcfc6c6c3ec32

>   src/master/allocator/mesos/metrics.hpp d04e9a11c77b6fb2d522608e3085f81f8a6657ca 
>   src/master/allocator/mesos/metrics.cpp 46dd7f7b3d8d49c59b42e789fe5dcd49f1693ace 
>   src/tests/hierarchical_allocator_tests.cpp 459e02576f6d05abbbcc83ae5cabac5c66e93f05

> 
> Diff: https://reviews.apache.org/r/43884/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