mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 43884: Added allocator metrics for used quotas.
Date Fri, 18 Mar 2016 16:08:41 GMT


> On March 17, 2016, 10:07 p.m., Ben Mahler wrote:
> > src/master/allocator/mesos/metrics.cpp, line 73
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299851#file1299851line73>
> >
> >     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;
> >     ```

I introduced this variable so I wouldn't have to capture the whole `Resource` by value for
the lambda below. Since I am moving away from using a lambda we can just use `resource.name()`
directly.


> On March 17, 2016, 10:07 p.m., Ben Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1694-1696
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299849#file1299849line1694>
> >
> >     How about the folllowing wrapping:
> >     
> >     ```
> >       Option<Value::Scalar> used =
> >         quotaRoleSorter->allocationScalarQuantities(role)
> >           .get<Value::Scalar>(resource);
> >     ```

Done, I added two more spaces before `.get`.


> On March 17, 2016, 10:07 p.m., Ben Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1698
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299849#file1299849line1698>
> >
> >     Recall that we have an `->` operator now to avoid the `.get().` pattern:
> >     
> >     ```
> >     return used.isSome() ? used->value() : 0;
> >     ```

Great to hear that this is the pattern we can follow now.


> On March 17, 2016, 10:07 p.m., Ben Mahler wrote:
> > src/master/allocator/mesos/metrics.cpp, lines 59-64
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299851#file1299851line59>
> >
> >     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);
> >       }
> >     }
> >     ```

Done.


> On March 17, 2016, 10:07 p.m., Ben Mahler wrote:
> > src/master/allocator/mesos/metrics.cpp, lines 68-87
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299851#file1299851line68>
> >
> >     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).

Done.


> On March 17, 2016, 10:07 p.m., Ben Mahler wrote:
> > src/master/allocator/mesos/metrics.cpp, lines 79-81
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299851#file1299851line79>
> >
> >     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
:)

I changed the member function signatures enabling us to use the preferred way to defer member
function calls. This also allowed me to change `Metrics` to hold a `PID<HierarchicalAllocatorProcess>`
simplifying the ownership semantics.


> On March 17, 2016, 10:07 p.m., Ben Mahler wrote:
> > src/master/allocator/mesos/metrics.hpp, lines 47-50
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299850#file1299850line47>
> >
> >     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'.

I felt this was desirable to include since we were holding a raw `HierarchicalAllocatorProcess*`.
Since we now hold a `PID<HierarchicalAllocatorProcess>` there is no need anymore to
lock the `Metrics` class down.


> On March 17, 2016, 10:07 p.m., Ben Mahler wrote:
> > src/master/allocator/mesos/metrics.hpp, lines 41-42
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299850#file1299850line41>
> >
> >     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?

Like you wrote below I couldn't `defer` using `PID` since my member functions where `const`
an no suitable overload for that exists for `defer`. I made the called member functions non-const
allowing us to hold a `PID` directly simplifying the situation.


> On March 17, 2016, 10:07 p.m., Ben Mahler wrote:
> > src/master/allocator/mesos/metrics.hpp, lines 35-38
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299850#file1299850line35>
> >
> >     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/".
> >     ```

Done.


> On March 17, 2016, 10:07 p.m., Ben Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1692
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299849#file1299849line1692>
> >
> >     Can you wrap each argument on its own line? That would be more consistent with
the wrapping we usually do for function signatures.

Done.


> On March 17, 2016, 10:07 p.m., Ben Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1691-1692
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299849#file1299849line1691>
> >
> >     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
> >     $
> >     ```

Done.


> On March 17, 2016, 10:07 p.m., Ben Mahler wrote:
> > src/master/allocator/mesos/hierarchical.hpp, lines 290-291
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299848#file1299848line290>
> >
> >     Could you wrap the arguments each on their own line to be more consistent with
our typical signature wrapping?

Done.


> On March 17, 2016, 10:07 p.m., Ben Mahler wrote:
> > src/master/allocator/mesos/metrics.cpp, line 70
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299851#file1299851line70>
> >
> >     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.

Done.


> On March 17, 2016, 10:07 p.m., Ben Mahler wrote:
> > src/master/allocator/mesos/metrics.cpp, lines 92-99
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299851#file1299851line92>
> >
> >     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);
> >     ```

Yes, this reads much nicer.


> On March 17, 2016, 10:07 p.m., Ben Mahler wrote:
> > src/master/allocator/mesos/metrics.cpp, line 86
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299851#file1299851line86>
> >
> >     How about using the [] operator? It tends to make the key and value read more
easily:
> >     
> >     ```
> >     quota_allocated[role] = gauges;
> >     ```

Note that this syntax is not possible a few lines above since a `Gauge` doesn't have a default
ctr, and `put` is the only option there. My original motivation was to use a single style
to add entries to the various maps. I am changing this one instance here as you suggested.


> On March 17, 2016, 10:07 p.m., Ben Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 1660-1664
> > <https://reviews.apache.org/r/43884/diff/16/?file=1299852#file1299852line1660>
> >
> >     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

I `s/metricKey/metric/g`.

The reason I am not checking the values here is that I explicitly check for entry absence;
would you rather have me check obtained values against `JSON::Null`?

To update the code below to use `contains` I'd have to adapt your suggestion. `contains` is
a member function of `JSON::Value` while `Metrics` returns a `JSON::Object`, so to use `contains`
we need to create a `JSON::Value` somewhere. Since we cannot use `contains` to check for value
absence we still need access to the `JSON::Object`'s `values`. If we'd choose to create the
temporary just before calling `contains` instead of e.g., using `JSON::Value metrics = Metrics();`
so we would not need to `ASSERT` that `metrics` `is<JSON::Object>`, the resulting code
would read

    JSON::Object metrics = Metrics();
    JSON::Object expected;
    expected.values = {
        {"allocator/mesos/quota/" + QUOTA_ROLE + "/cpus_allocated", 0.25},
        {"allocator/mesos/quota/" + QUOTA_ROLE + "/mem_allocated", 128}};
    EXPECT_TRUE(JSON::Value(metrics).contains(expected));
    string metric = "allocator/mesos/quota/" + QUOTA_ROLE + "/disk_allocated";
    EXPECT_EQ(0u, metrics.values.count(metric));

as opposed to originally

    JSON::Object metrics = Metrics();
    string metricKey = "allocator/mesos/quota/" + QUOTA_ROLE + "/cpus_allocated";
    EXPECT_EQ(0.25, metrics.values[metricKey]);
    metricKey = "allocator/mesos/quota/" + QUOTA_ROLE + "/mem_allocated";
    EXPECT_EQ(128, metrics.values[metricKey]);
    metricKey = "allocator/mesos/quota/" + QUOTA_ROLE + "/disk_allocated";
    EXPECT_EQ(0u, metrics.values.count(metricKey));

I have the feeling the changed version requires more mental effort to parse.


- Benjamin


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


On March 18, 2016, 5:08 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43884/
> -----------------------------------------------------------
> 
> (Updated March 18, 2016, 5:08 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 0c622b8569bc79ae4e365a57e463ca5349356713

>   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