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, 25 Jul 2017 22:06:14 GMT


> On July 18, 2017, 2:16 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/fetcher.cpp
> > Lines 272-275 (patched)
> > <https://reviews.apache.org/r/60791/diff/3/?file=1778079#file1778079line272>
> >
> >     Aside from styling/convention, would this require defer?
> 
> James Peach wrote:
>     No, this is avoiding the defer on purpose.
> 
> Jiang Yan Xu wrote:
>     So `tally` is concurrently modified by another thead, is the reason this is thread-safe
that `Bytes` is a simple class with one `uint64_t` field? Even in this case is it always safe?
Is there a definitive reference that could help me be convinced?
>     
>     Even if it is the case I think this could be subtle. I thought the original intention
for /r/59858/ is for constants, which would be more easy to reason about correctness?
>     
>     In terms of the performance gains, I am not sure it's worthwhile make an exception
here given this is on the agent and thus not a performance bottleneck. (I am not against something
that can be proposed as a general recommendations which we can adopt for more than one use.)
>     
>     To extend from this, is metrics collection special? So far all concurrent accesses
to Process internals are protected by dispatches, what to do about those?
> 
> James Peach wrote:
>     Going forward I think that we should avoid the `defer` in cases where it is safe
to do so since metrics collection should be as light as possible. We should encourage contributors
to design metrics in such a way that they can safely be sampled without locking or adding
more work to the process.

So James and I had a offline dicussion and it should be safe to read a `Bytes` value while
it is being concurrently modified. This coupled with awaiting `process::metrics::remove` to
finish in destructor makes it an acceptable solution with the current `Gauage` API that takes
a `std::function`. Therefore let's ship it first.

However I think we should explore a bit further about the `Gauge` API. If a major use case
for passing a plain `std::function` is to read some simple variables without deferring, then
shouldn't the API/usage support it natively? i.e., can we register this variable in the metrics
process for automatic retrieval?

Perhpas worthy of a discussion in the #performance WG?


- Jiang Yan


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


On July 18, 2017, 8:05 p.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60791/
> -----------------------------------------------------------
> 
> (Updated July 18, 2017, 8:05 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_bytes`
> and the used space is `containerizer/fetcher/cache_size_used_bytes`.
> 
> 
> Diffs
> -----
> 
>   docs/monitoring.md 38b8093ef683b5de65cbfa5330a6bbd1c9e10f12 
>   src/slave/containerizer/fetcher.cpp ad067c3158d274fd4a39436ab8bf22dceaf3fb54 
>   src/slave/containerizer/fetcher_process.hpp 3ed7dc9db5b64b72881249767c0356a3bc5370e7

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


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