----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61260/#review181849 -----------------------------------------------------------
src/slave/gc.cpp Lines 50 (patched) <https://reviews.apache.org/r/61260/#comment257625> s/total/completed/? Just to distinguish from the ones that are unscheduled/cancelled? Also when comparing `gc_path_removals_total` and `gc_path_removals_scheduled` it's not clear if the former is a superset of the latter. src/slave/gc.cpp Lines 52-57 (patched) <https://reviews.apache.org/r/61260/#comment257624> The name `_scheduled` is a bit ambiguous as it can be interpreted as a Counter that tracks all paths that have ever been scheduled. (The API is called `schedule()`). "The number of paths that have ever been scheduled" is probably itself an interesting metric. Can we add it? This current metric could be named "slave/gc_path_removals_pending" src/slave/gc.cpp Lines 55 (patched) <https://reviews.apache.org/r/61260/#comment257619> s/ it// src/slave/gc.cpp Lines 199 (patched) <https://reviews.apache.org/r/61260/#comment257616> Generally prefer pre-increments for object types: https://google.github.io/styleguide/cppguide.html#Preincrement_and_Predecrement Also, strictly speaking this labmda can be run after the GC is destructed. Seems like we have to pass in the two counters as copies directly. src/slave/gc.cpp Lines 205 (patched) <https://reviews.apache.org/r/61260/#comment257617> Ditto. src/tests/gc_tests.cpp Lines 95 (patched) <https://reviews.apache.org/r/61260/#comment257637> s/count/expected/? Also I don't see too much value in this class-level helper. It may be reducing `ASSERT_EQ` and `EXPECT_SOME_EQ` into one call but the `JSON::Object metrics = Metrics();` line is already shared when multiple metrics are checked; and it is more correct semantically. i.e., we are reading the metrics once and verifying the consistency between the metrics. One could argue that we know in this test the metrics don't change between invocations of this helper but this is at the expense of clarity. Overall I think we can either 1) continue to inline these or 2) if it makes a strong case for an abstraction, provide it as a more general helper (possibly put it where `JSON::Object Metrics()` is defined). - Jiang Yan Xu On July 31, 2017, 9:31 a.m., James Peach wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61260/ > ----------------------------------------------------------- > > (Updated July 31, 2017, 9:31 a.m.) > > > Review request for mesos and Jiang Yan Xu. > > > Bugs: MESOS-7842 > https://issues.apache.org/jira/browse/MESOS-7842 > > > Repository: mesos > > > Description > ------- > > Added some basic sandbox garbage collection metrics to track the number > of paths removals, the number of failed path removal and the number of > paths that are still scheduled for removal. > > > Diffs > ----- > > src/slave/gc.cpp f270fa47b22904e926f4bd4b0a7693a41b2c8271 > src/slave/gc_process.hpp PRE-CREATION > src/tests/gc_tests.cpp d4daad3993bb9a92db2c7add6e74f12aeb71d535 > > > Diff: https://reviews.apache.org/r/61260/diff/1/ > > > Testing > ------- > > make check (Fedora 26). > > > Thanks, > > James Peach > >
