mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ben Mahler" <benjamin.mah...@gmail.com>
Subject Re: Review Request 37417: Convert Perf event validator to use new shared object.
Date Tue, 18 Aug 2015 20:38:10 GMT

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


Paul can you split this change? I personally don't have context on why we would want to change
from sampling 'true' to running against the init process so I'd like Jie or Ian to review
that change, but I can help you make the refactorings to use 'Perf'. Sound good?


src/linux/perf.cpp (lines 395 - 408)
<https://reviews.apache.org/r/37417/#comment150891>

    Looks like change is doing more than just using the Perf object. You've also changed this
to work off of init rather than the 'true' command? Why the latter change? (Not clear from
the review description or ticket)
    
    Also, any reason you introduce internal::validate()? Looks like it can just be inlined
inside valid()?



src/linux/perf.cpp (lines 464 - 467)
<https://reviews.apache.org/r/37417/#comment150892>

    Let's try to avoid jaggedness in comments :)



src/linux/perf.cpp (lines 473 - 474)
<https://reviews.apache.org/r/37417/#comment150894>

    future.await() is an anti-pattern, so let's explain why it's needed here. I assume it's
because isolator creation is synchronous currently and making it asynchronous is a larger
undertaking?



src/linux/perf.cpp (line 476)
<https://reviews.apache.org/r/37417/#comment150893>

    Please remember to put spaces between your if and open paren. Ideally this should be automated,
but it's not right now :)


- Ben Mahler


On Aug. 18, 2015, 6:43 p.m., Paul Brett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37417/
> -----------------------------------------------------------
> 
> (Updated Aug. 18, 2015, 6:43 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3185
>     https://issues.apache.org/jira/browse/MESOS-3185
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Convert Perf event validator to use new shared object.
> 
> 
> Diffs
> -----
> 
>   src/linux/perf.cpp cb1a13d0b1754a50f0121bfda522056ff8c3e3c8 
> 
> Diff: https://reviews.apache.org/r/37417/diff/
> 
> 
> Testing
> -------
> 
> sudo make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>


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