mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ian Downes" <ian.dow...@gmail.com>
Subject Re: Review Request 38074: Calculate schedule latency with trace events
Date Wed, 18 Nov 2015 00:51:32 GMT

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



include/mesos/mesos.proto (line 907)
<https://reviews.apache.org/r/38074/#comment165762>

    s/percentile/percentiles/



src/slave/containerizer/isolators/cgroups/perf_event.hpp (line 97)
<https://reviews.apache.org/r/38074/#comment165768>

    linux/cgroups has an internal {{Result<string> cgroup(pid_t pid, const string&
subsystem)}} that is wrapped for cpu, cpuacct, etc. Can you add an wrapper for the perf_event
cgroup and use that? Yes, it may be less efficient than looking at the hashmap, but it'll
reuse the code and keep consistency.



src/slave/containerizer/isolators/cgroups/perf_event.hpp (line 140)
<https://reviews.apache.org/r/38074/#comment165769>

    Format to 80 cols, please.



src/slave/containerizer/isolators/cgroups/perf_event.cpp (lines 116 - 118)
<https://reviews.apache.org/r/38074/#comment165773>

    What is this conditional on? It looks like any use of the perf event isolator will enable
the scheduler latency functionality...



src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 130)
<https://reviews.apache.org/r/38074/#comment165795>

    const string&



src/slave/containerizer/isolators/cgroups/perf_event.cpp (lines 466 - 476)
<https://reviews.apache.org/r/38074/#comment165774>

    remove this, see earlier comment.



src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 480)
<https://reviews.apache.org/r/38074/#comment165794>

    Suggest that you use a hashmap<pid_t, string>, i.e., just directly store the cgroup
for each pid. Slight tradeoff in size but then you can avoid findCgroupByPid() and simplify
the code.



src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 482)
<https://reviews.apache.org/r/38074/#comment165775>

    const string&



src/slave/containerizer/isolators/cgroups/perf_event.cpp (lines 483 - 484)
<https://reviews.apache.org/r/38074/#comment165776>

    Please clarify the language, is it "should not" or "does not" :-)



src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 485)
<https://reviews.apache.org/r/38074/#comment165778>

    const Future<>&



src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 486)
<https://reviews.apache.org/r/38074/#comment165779>

    Is each Future guaranteed to be ready?



src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 487)
<https://reviews.apache.org/r/38074/#comment165780>

    Please clarify language, "should" or "is"?



src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 489)
<https://reviews.apache.org/r/38074/#comment165781>

    Call this variable child?



src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 490)
<https://reviews.apache.org/r/38074/#comment165782>

    We don't use CHECKs in code unless it truly is an unrecoverable error. Please fail the
future or act appropriately.



src/slave/containerizer/isolators/cgroups/perf_event.cpp (lines 491 - 492)
<https://reviews.apache.org/r/38074/#comment165785>

    No snake_case please, here and everywhere.
    
    Use C++ cast, not C style.



src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 492)
<https://reviews.apache.org/r/38074/#comment165786>

    ditch the intermediate variable and just insert the cast child.get()?
    
    and, using my suggestion above, processes[pid] = cgroup



src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 496)
<https://reviews.apache.org/r/38074/#comment165797>

    You don't actually use this as a map? It would be much clearer to use a vector and explicitly
sort with a comparision function on the timestamp. Suggest just calling it events.
    
    ```
      events.push_back(event);
    }
    
    std::sort(begin(events), end(events), [](...){});
    ```



src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 506)
<https://reviews.apache.org/r/38074/#comment165798>

    const TraceEvent&



src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 514)
<https://reviews.apache.org/r/38074/#comment165814>

    It would be nice to have helpers which returned the correct type for event fields.



src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 515)
<https://reviews.apache.org/r/38074/#comment165787>

    No CHECKs.



src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 516)
<https://reviews.apache.org/r/38074/#comment165788>

    C++ cast.



src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 526)
<https://reviews.apache.org/r/38074/#comment165789>

    ditto.



src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 527)
<https://reviews.apache.org/r/38074/#comment165790>

    ditto.



src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 529)
<https://reviews.apache.org/r/38074/#comment165791>

    ditto.



src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 530)
<https://reviews.apache.org/r/38074/#comment165792>

    ditto.



src/slave/containerizer/isolators/cgroups/perf_event.cpp (lines 536 - 537)
<https://reviews.apache.org/r/38074/#comment165800>

    Just continue if pid == 0, rather than nesting this conditional block on != 0.



src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 539)
<https://reviews.apache.org/r/38074/#comment165793>

    ditto.



src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 541)
<https://reviews.apache.org/r/38074/#comment165801>

    Are states enumerated somewhere?



src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 544)
<https://reviews.apache.org/r/38074/#comment165802>

    newline between blocks



src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 554)
<https://reviews.apache.org/r/38074/#comment165804>

    sort the latencies in schedLatency here?



src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 556)
<https://reviews.apache.org/r/38074/#comment165803>

    const string&
    
    grab the cgroup and vector together rather than indexing repeatedly...
    
    ```
    foreachpair(const string& cgroup,
                const vector<uint64_t>& latencies,
                schedLatency)
    ```



src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 606)
<https://reviews.apache.org/r/38074/#comment165805>

    why not call the variable threads?



src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 608)
<https://reviews.apache.org/r/38074/#comment165806>

    also log the error, {{process.error()}}.



src/slave/containerizer/isolators/cgroups/perf_event.cpp (lines 608 - 609)
<https://reviews.apache.org/r/38074/#comment165815>

    Could you just skip rather than failing for all cgroups?



src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 611)
<https://reviews.apache.org/r/38074/#comment165810>

    as above, suggest stored pid -> cgroup mapping denormalized.



src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 615)
<https://reviews.apache.org/r/38074/#comment165809>

    const string&
    The key is an event? If so, it should be named as such.



src/slave/containerizer/isolators/cgroups/perf_event.cpp (lines 619 - 621)
<https://reviews.apache.org/r/38074/#comment165807>

    How expensive is this...?



src/slave/containerizer/isolators/cgroups/perf_event.cpp (lines 623 - 626)
<https://reviews.apache.org/r/38074/#comment165808>

    Users have different kernels, code should determine the version at run time and act accordingly.



src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 633)
<https://reviews.apache.org/r/38074/#comment165811>

    Include error message _events.error().



src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 636)
<https://reviews.apache.org/r/38074/#comment165812>

    const string&.
    
    I don't think there's another cgroup variable in scope so just use cgroup, not _cgroup.


- Ian Downes


On Sept. 29, 2015, 5:15 p.m., Cong Wang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38074/
> -----------------------------------------------------------
> 
> (Updated Sept. 29, 2015, 5:15 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-2769
>     https://issues.apache.org/jira/browse/MESOS-2769
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Finally, calculate schedule latency with the sched trace events, and add it to our statistics
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 4a16be1f570769f3ce42a50a9da9f4fb1c227999 
>   src/slave/containerizer/isolators/cgroups/perf_event.hpp 1f722ef3ef7ab7fce5542d4affae961d6cec2406

>   src/slave/containerizer/isolators/cgroups/perf_event.cpp 03035dfbfb84470ba39ed9ecfd1eb73890e3f784

>   src/slave/flags.hpp f76f0f6a0c7a01c7f061a225d7f6ef52be0ee7b5 
>   src/slave/flags.cpp 029aa1eb00e1aa3e92c2155925022c17fd905862 
> 
> Diff: https://reviews.apache.org/r/38074/diff/
> 
> 
> Testing
> -------
> 
> manual tests
> 
> 
> Thanks,
> 
> Cong Wang
> 
>


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