mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 61260: Added agent garbage collection metrics.
Date Mon, 31 Jul 2017 23:26:04 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message