> On July 18, 2017, 9: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?
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. - James ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60791/#review180862 ----------------------------------------------------------- On July 19, 2017, 3:05 a.m., James Peach wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60791/ > ----------------------------------------------------------- > > (Updated July 19, 2017, 3:05 a.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 > 6a664e0657a19d27afac98fd5298d6a18aabe43f > src/slave/containerizer/fetcher_process.hpp > 3ed7dc9db5b64b72881249767c0356a3bc5370e7 > src/tests/fetcher_cache_tests.cpp 1c654e511d2079de746ac97a2c2718e1b926768e > > > Diff: https://reviews.apache.org/r/60791/diff/4/ > > > Testing > ------- > > make check (Fedora 26). > > > Thanks, > > James Peach > >
