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 60791: Add fetcher cache space usage metrics.
Date Tue, 18 Jul 2017 21:16:41 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60791/#review180862
-----------------------------------------------------------



Haven't looked at tests but perhaps we can discuss the current issues first.


docs/monitoring.md
Lines 1258-1270 (patched)
<https://reviews.apache.org/r/60791/#comment256166>

    We do have a lot of disk related metrics in "bytes".
    
    Inconsisteny does exist but the line is strictly between the "abstract" resources and
"physical" resources. 
    
    All "abstract" disk/memory resources currently don't have units in the name and are in
MBs. This is because the flags and resource quantity calculation in Mesos are in MBs. Not
having the unit in the name is bad IMO but many of these are historical.
    
    The "physical" resource metrics all have units and are in bytes:
    
    ```
    disk_limit_bytes
    disk_used_bytes
    system/mem_free_bytes
    system/mem_free_bytes
    ```
    
    So despiste the inconsistency (which is not going to be fixed by using MB here), I think
we should use bytes here and have the unit explicit in the metric name because it's clearer.
    
    If we make things consistent, I would vote to have units with all resource metrics.



src/slave/containerizer/fetcher.cpp
Lines 266-267 (original), 266-275 (patched)
<https://reviews.apache.org/r/60791/#comment256174>

    Outside the fetcher I think we use a convention like this: https://github.com/apache/mesos/blob/d5b85d2b9063b5fb4a40108ec4801cc5ed2f5155/src/master/registrar.cpp#L146-L195
    
    i.e., 
    
    - Group metrics in a `Metrics` nested class. (Later when you have more of them and would
like to pull them out into a different file you can easily do so).
    - Snake casing which is consistent with the metric name.
    - For gauges, defer to an internal private handler prefixed by a `_`.
    
    Any reason to diverge?
    
    (Aside from `cache_size_total` which is const)



src/slave/containerizer/fetcher.cpp
Lines 272-275 (patched)
<https://reviews.apache.org/r/60791/#comment256171>

    Aside from styling/convention, would this require defer?



src/slave/containerizer/fetcher.cpp
Lines 293-295 (patched)
<https://reviews.apache.org/r/60791/#comment256172>

    Why await?



src/slave/containerizer/fetcher_process.hpp
Lines 147-148 (patched)
<https://reviews.apache.org/r/60791/#comment256173>

    If these are used only for monitoring, should we just follow the convention mentioned
above and make them private?


- Jiang Yan Xu


On July 17, 2017, 7:13 p.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60791/
> -----------------------------------------------------------
> 
> (Updated July 17, 2017, 7:13 p.m.)
> 
> 
> Review request for mesos, Joseph Wu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7782
>     https://issues.apache.org/jira/browse/MESOS-7782
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add fetcher metrics to track the (constant) size of the cache size
> and the (varying) amount of cache space use. The cache size is
> published as `containerizer/fetcher/cache_size_total` and the used
> space is `containerizer/fetcher/cache_size_used`. Both of these
> metrics are in MB to be consistent with other disk space metrics.
> We manually convert to fractional MB so that small amounts of space
> usage don't get truncated away.
> 
> 
> Diffs
> -----
> 
>   docs/monitoring.md 38b8093ef683b5de65cbfa5330a6bbd1c9e10f12 
>   src/slave/containerizer/fetcher.cpp 6a664e0657a19d27afac98fd5298d6a18aabe43f 
>   src/slave/containerizer/fetcher_process.hpp 3ed7dc9db5b64b72881249767c0356a3bc5370e7

>   src/tests/fetcher_cache_tests.cpp 1c654e511d2079de746ac97a2c2718e1b926768e 
> 
> 
> Diff: https://reviews.apache.org/r/60791/diff/3/
> 
> 
> Testing
> -------
> 
> make check (Fedora 26).
> 
> 
> Thanks,
> 
> James Peach
> 
>


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