mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jie Yu" <yujie....@gmail.com>
Subject Re: Review Request 36378: Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.
Date Fri, 17 Jul 2015 21:57:00 GMT

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


Thanks Paul for taking this on!

See my detailed comments. We should first pulling the subprocess logic into 'perf::execute'
first. Then, we can add version support in the subsequent patch. I haven't looked at the version
stuff yet.


src/linux/perf.cpp (line 162)
<https://reviews.apache.org/r/36378/#comment146076>

    Kill this line.



src/linux/perf.cpp (line 163)
<https://reviews.apache.org/r/36378/#comment146071>

    s/commandResults/Output.



src/linux/perf.cpp (line 165)
<https://reviews.apache.org/r/36378/#comment146072>

    s/exitCode/status/



src/linux/perf.cpp (line 166)
<https://reviews.apache.org/r/36378/#comment146073>

    s/stdOut/out/



src/linux/perf.cpp (lines 166 - 167)
<https://reviews.apache.org/r/36378/#comment146075>

    Do you need std:: prefix in a cpp file? Can you just use 'using std::string' in the begining?
    
    Please do a sweep to cleanup all the occurances in this file.



src/linux/perf.cpp (line 167)
<https://reviews.apache.org/r/36378/#comment146074>

    s/stdErr/err/



src/linux/perf.cpp (lines 171 - 173)
<https://reviews.apache.org/r/36378/#comment146070>

    Pulling this out is great! But can we have a patch first to do this refactor first, and
then add the 'version' stuff in the second patch?



src/linux/perf.cpp (lines 171 - 173)
<https://reviews.apache.org/r/36378/#comment146077>

    I would s/executeCommand/execute/ since you already have a 'cmd' in the argument list.
    
    I would rather make this function specific to perf. For example: (no need the 'cmd')
    
    When MESOS-3035 gets in, you can just modify perf::execute implementation to use that
instead.
    
    ```
    perf::execute({"--version"});
    perf::execute({"stat", "--all-cpus"});
    ....
    ```



src/linux/perf.cpp (line 178)
<https://reviews.apache.org/r/36378/#comment146080>

    s/execCmd/s/



src/linux/perf.cpp (lines 179 - 183)
<https://reviews.apache.org/r/36378/#comment146079>

    Please fix the indentation.



src/linux/perf.cpp (line 181)
<https://reviews.apache.org/r/36378/#comment146078>

    Do you need 'stdin'? Should that be Subprocess::PATH("/dev/null")?



src/linux/perf.cpp (line 189)
<https://reviews.apache.org/r/36378/#comment146081>

    No need for this temp variable.



src/linux/perf.cpp (lines 191 - 207)
<https://reviews.apache.org/r/36378/#comment146084>

    As Ben suggested in the email thread, you can do:
    
    ```
    process::await(
        perf.get().status(),
        io::read(perf.get().out().get()),
        io::read(perf.get().err().get()))
      .then([](...) -> Future<Output> { ... });
    ```
    
    You don't need the lambda capture anymore if you wrote code like above. Please take a
look at https://github.com/apache/mesos/blob/master/src/slave/containerizer/isolators/posix/disk.cpp#L428



src/linux/perf.cpp (line 203)
<https://reviews.apache.org/r/36378/#comment146087>

    I would rather let the caller dealing with extracting exit code. So returning 'status.get()'
is better.



src/linux/perf.cpp (lines 358 - 383)
<https://reviews.apache.org/r/36378/#comment146089>

    Do you want to simplify this as well?
    
    I think 'perf::execute' should use 'setupChild' as well. See the following for details:
    
    https://issues.apache.org/jira/browse/MESOS-2462
    https://reviews.apache.org/r/32805/


- Jie Yu


On July 17, 2015, 1:30 a.m., Paul Brett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36378/
> -----------------------------------------------------------
> 
> (Updated July 17, 2015, 1:30 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
> 
> 
> Bugs: MESOS-2834
>     https://issues.apache.org/jira/browse/MESOS-2834
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending
on kernel version.
> 
> 
> Diffs
> -----
> 
>   src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 
> 
> Diff: https://reviews.apache.org/r/36378/diff/
> 
> 
> Testing
> -------
> 
> sudo make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>


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