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 Thu, 20 Jul 2017 07:48:03 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.

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?


> On July 18, 2017, 2:16 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/fetcher.cpp
> > Lines 293-295 (patched)
> > <https://reviews.apache.org/r/60791/diff/3/?file=1778079#file1778079line293>
> >
> >     Why await?
> 
> James Peach wrote:
>     This synchronizes the metric removal so that it can't be evaluated after the process
has been destroyed.

This `await` doesn't block? `Future::await` would block.


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