This is an automatically generated e-mail. To reply, visit:

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

docs/monitoring.md (lines 876 - 883)

    Thanks! How about:
      <td>Amount of resources considered allocated towards a role's quota 
    Also, can you add the guarantee to this patch?
      <td>Amount of resources guaranteed for a role via quota</td>
    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:

src/master/allocator/mesos/hierarchical.hpp (lines 290 - 291)

    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)

    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)

    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)

    How about the folllowing wrapping:
      Option<Value::Scalar> used =

src/master/allocator/mesos/hierarchical.cpp (line 1698)

    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)

    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 
    // Collection of metrics for the allocator, these begin with
    // the following prefix: "allocator/mesos/".

src/master/allocator/mesos/metrics.hpp (lines 41 - 42)

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

    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)

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

src/master/allocator/mesos/metrics.cpp (lines 68 - 87)

    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)

    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)

    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() + 
      gauges[resource.name()] = gauge;

src/master/allocator/mesos/metrics.cpp (lines 79 - 81)

    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)

    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)

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

src/tests/hierarchical_allocator_tests.cpp (lines 1660 - 1664)

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

Reply via email to