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