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