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

Reply via email to