You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Paul Brett <pa...@twopensource.com> on 2015/09/01 00:14:04 UTC

Re: Review Request 37416: Perf supported() should be based on the version of perf, not the version of the kernel.

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

(Updated Aug. 31, 2015, 10:14 p.m.)


Review request for mesos and Ben Mahler.


Repository: mesos


Description
-------

Perf supported() should be based on the version of perf, not the version of the kernel.


Diffs (updated)
-----

  src/linux/perf.cpp cdc5f8314a875ababf2b17a32873775d808e1c78 

Diff: https://reviews.apache.org/r/37416/diff/


Testing
-------

sudo make check


Thanks,

Paul Brett

Re: Review Request 37416: Perf supported() should be based on the version of perf, not the version of the kernel.

Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37416/#review97172
-----------------------------------------------------------

Ship it!


Some stuff leftover from the last review comments, I'll make the updates and commit for you shortly.


src/linux/perf.cpp (line 393)
<https://reviews.apache.org/r/37416/#comment152961>

    two space indent for .then



src/linux/perf.cpp (line 474)
<https://reviews.apache.org/r/37416/#comment152962>

    if (



src/linux/perf.cpp (lines 474 - 482)
<https://reviews.apache.org/r/37416/#comment152965>

    Hm.. you didn't add the explicit discard here?
    
    ```
      if (!version.isReady()) {
        if (version.isFailed()) {
          LOG(ERROR) << "Failed to get perf version: " << version.failure();
        } else {
          LOG(ERROR) << "Failed to get perf version: timeout of 5secs exceeded";
        }
    
        version.discard();
        return false;
      }
    ```



src/linux/perf.cpp (line 478)
<https://reviews.apache.org/r/37416/#comment152964>

    This case also needs to return!



src/linux/perf.cpp (line 479)
<https://reviews.apache.org/r/37416/#comment152963>

    extra space here?


- Ben Mahler


On Aug. 31, 2015, 10:27 p.m., Paul Brett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37416/
> -----------------------------------------------------------
> 
> (Updated Aug. 31, 2015, 10:27 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Perf supported() should be based on the version of perf, not the version of the kernel.
> 
> 
> Diffs
> -----
> 
>   src/linux/perf.cpp cdc5f8314a875ababf2b17a32873775d808e1c78 
> 
> Diff: https://reviews.apache.org/r/37416/diff/
> 
> 
> Testing
> -------
> 
> sudo make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 37416: Perf supported() should be based on the version of perf, not the version of the kernel.

Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37416/
-----------------------------------------------------------

(Updated Aug. 31, 2015, 10:27 p.m.)


Review request for mesos and Ben Mahler.


Repository: mesos


Description
-------

Perf supported() should be based on the version of perf, not the version of the kernel.


Diffs
-----

  src/linux/perf.cpp cdc5f8314a875ababf2b17a32873775d808e1c78 

Diff: https://reviews.apache.org/r/37416/diff/


Testing
-------

sudo make check


Thanks,

Paul Brett

Re: Review Request 37416: Perf supported() should be based on the version of perf, not the version of the kernel.

Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37416/
-----------------------------------------------------------

(Updated Aug. 31, 2015, 10:27 p.m.)


Review request for mesos and Ben Mahler.


Changes
-------

Rebased


Repository: mesos


Description
-------

Perf supported() should be based on the version of perf, not the version of the kernel.


Diffs (updated)
-----

  src/linux/perf.cpp cdc5f8314a875ababf2b17a32873775d808e1c78 

Diff: https://reviews.apache.org/r/37416/diff/


Testing
-------

sudo make check


Thanks,

Paul Brett