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