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

Reply via email to