mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From James Peach <jpe...@apache.org>
Subject Re: Review Request 60791: Add fetcher cache space usage metrics.
Date Fri, 21 Jul 2017 22:48:19 GMT


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


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