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 Tue, 01 Sep 2015 23:43:34 GMT

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


Looks good! Should be shippable after another round.


src/linux/perf.hpp (line 56)
<https://reviews.apache.org/r/37462/#comment153245>

    Whoops, doesn't this comment belong on the parse function?



src/linux/perf.cpp (lines 278 - 281)
<https://reviews.apache.org/r/37462/#comment153244>

    Hm.. can't we just add this as an overload of supported?
    
    ```
    bool supported();
    bool supported(const Version& version);
    ```
    
    The first being only in the .cpp file, and let's stick it right below the existing supported
to make the relationship clear.



src/linux/perf.cpp (lines 324 - 353)
<https://reviews.apache.org/r/37462/#comment153247>

    To keep it clean, how about we just do the supported check initially, and avoid the 'check'
lambda entirely:
    
    ```
    if (!supported()) {
      return Failure("Perf is not supported");
    }
    
    internal::Perf* perf = new internal::Perf(argv);
    Future<string> output = perf->output();
    spawn(perf, true);
    
    auto parse = [start, duration](
        const Version& version,
        const string& output) ->
            Future<hashmap<string, mesos::PerfStatistics>> {
      Try<hashmap<string, mesos::PerfStatistics>> result =
        perf::parse(output, version);
    
      if (result.isError()) {
        return Failure("Failed to parse perf sample: " + result.error());
      }
    
      foreachvalue (mesos::PerfStatistics& statistics, result.get()) {
        statistics.set_timestamp(start.secs());
        statistics.set_duration(duration.secs());
      }
    
      return result.get();
    };
    
    process::collect(perf::version(), output)
      .then(parse);
    ```
    
    Note that there are races here either way, where the version we check might not match
the version of perf that provided the sample.
    
    These lambdas are getting ugly :(



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

    Let's just make an overload of supported that takes a version, see my comment above.



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

    Let's just omit these shas since we can just look at the tags instead:
    
    https://github.com/torvalds/linux/blob/v4.0/tools/perf/perf.c
    
    Ditto below.



src/linux/perf.cpp (lines 410 - 413)
<https://reviews.apache.org/r/37462/#comment153238>

    How about just showing the two formats, and above where we call split() just saying that
we use split because some fields may be empty (e.g. unit, see below). Seems a bit odd to show
the empty unit cases here as extra cases.



src/linux/perf.cpp (lines 414 - 417)
<https://reviews.apache.org/r/37462/#comment153234>

    Braces please :)
    
    Also, this can just be a single statement?
    
    ```
    if (tokens.size() == 4 || tokens.size() = 6) {
      ...
    }
    ```



src/linux/perf.cpp (lines 422 - 423)
<https://reviews.apache.org/r/37462/#comment153240>

    Braces please :)



src/linux/perf.cpp (lines 427 - 428)
<https://reviews.apache.org/r/37462/#comment153241>

    Braces please :)



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

    The caller will print the line, let's just say what's wrong here (i.e. unexpected number
of tokens?).
    
    Also let's add a newline above this.



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

    Do you have a tool that is removing spaces on if statements, or?



src/tests/containerizer/perf_tests.cpp 
<https://reviews.apache.org/r/37462/#comment153248>

    Why did this get removed?



src/tests/containerizer/perf_tests.cpp (line 69)
<https://reviews.apache.org/r/37462/#comment153249>

    Please wrap these onto the next line, ditto below.


- Ben Mahler


On Sept. 1, 2015, 8:45 p.m., Paul Brett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37462/
> -----------------------------------------------------------
> 
> (Updated Sept. 1, 2015, 8:45 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 dac7061471a0fa05de12cb530bcd5c63a6a71eee 
>   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