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 37462: Add support for version detection and parsing.
Date Wed, 02 Sep 2015 22:02:07 GMT

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

Ship it!


I've made adjustments based on the feedback below before committing, please take a look over
the comments to make sure they all make sense to you.


src/linux/perf.cpp (line 271)
<https://reviews.apache.org/r/37462/#comment153453>

    Missed this earlier, s/=//



src/linux/perf.cpp (line 287)
<https://reviews.apache.org/r/37462/#comment153449>

    Looks like this comment belongs in the overload now?



src/linux/perf.cpp (lines 348 - 351)
<https://reviews.apache.org/r/37462/#comment153458>

    If you use process::collect instead of process::await, this gets cleaned up substantially,
you can deal directly with the values rather than the transitioned Futures.



src/linux/perf.cpp (line 352)
<https://reviews.apache.org/r/37462/#comment153462>

    I'd suggest we just unpack the tuple at the top, otherwise it looks pretty verbose. Note
that this would be similar to adding an `unpacked` function wrapper as michael suggested here:
    
    https://issues.apache.org/jira/browse/MESOS-3188



src/linux/perf.cpp (lines 358 - 359)
<https://reviews.apache.org/r/37462/#comment153459>

    The reason should come after the colon, so this reads a bit strangely:
    
    Perf version unsupported: 2.6.38
    
    How about:
    
    Perf 2.6.38 is not supported



src/linux/perf.cpp (lines 383 - 384)
<https://reviews.apache.org/r/37462/#comment153521>

    Much like we don't wait for version and output forever in perf::supported, let's add a
TODO here to not wait forever:
    
    ```
    // TODO(pbrett): Don't wait for these forever.
    ```



src/linux/perf.cpp (line 384)
<https://reviews.apache.org/r/37462/#comment153454>

    Generally we try to put the .then on the next line, as if it was a statement:
    
    ```
      return process::collect(perf::version(), output)
        .then(parse);
    ```



src/linux/perf.cpp 
<https://reviews.apache.org/r/37462/#comment153447>

    Seems minor, but makes life easier for reviewers when code movement is pulled out in separate
patches as the diff is easier to read. :)



src/linux/perf.cpp (line 417)
<https://reviews.apache.org/r/37462/#comment153442>

    We already say this in the comment for this function, and it's shown in the call to split,
do we need to say it again?



src/linux/perf.cpp (lines 419 - 421)
<https://reviews.apache.org/r/37462/#comment153444>

    How about putting the format on a separate line as you did before?
    
    ```
          // Optional running time and ratio were introduced in Linux v4.0,
          // which make the format either:
          //   value,unit,event,cgroup
          //   value,unit,event,cgroup,running,ratio
    ```



src/linux/perf.cpp (lines 468 - 469)
<https://reviews.apache.org/r/37462/#comment153440>

    It's a little bit easier to avoid missing close quotes when it's on the same line:
    
    ```
    return Error("Unexpected event '" + sample->event + "'"
                 " in perf output at line: " + line);
    ```


- Ben Mahler


On Sept. 2, 2015, 6:52 p.m., Paul Brett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37462/
> -----------------------------------------------------------
> 
> (Updated Sept. 2, 2015, 6:52 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2834
>     https://issues.apache.org/jira/browse/MESOS-2834
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add support for version detection and parsing.
> 
> 
> Diffs
> -----
> 
>   src/linux/perf.hpp c44445630118f4858b1a805f25a2db7a24ca0989 
>   src/linux/perf.cpp bb8b5918e4c8f4379784d4fa18c3b4709696e1ea 
>   src/tests/containerizer/perf_tests.cpp 8c29ca3dc2a8f6d1fb0f430482190f87f08eb4e8 
> 
> Diff: https://reviews.apache.org/r/37462/diff/
> 
> 
> Testing
> -------
> 
> sudo make check
> 
> Perf tests may fail on many machines because the tests are currently using the version
of perf installed on the machine to choose decode.  The next patch will extend the perf tests
to supply test cases for each of the supported perf versions.
> 
> 
> Thanks,
> 
> Paul Brett
> 
>


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