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 45932: Add stripped TaskInfo's to ResourceUsage.Executor message.
Date Sat, 09 Apr 2016 00:27:52 GMT

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



Thanks! Looking pretty good, some minor comments below.


CHANGELOG (line 52)
<https://reviews.apache.org/r/45932/#comment191319>

    How about:
    
    ```
    Add task information to container resource usage information.
    ```



include/mesos/mesos.proto (lines 1026 - 1027)
<https://reviews.apache.org/r/45932/#comment191312>

    We should avoid saying "used" here in favor of "allocated". We can probably do without
this comment.
    
    How about s/resources/allocated here to match the executor above?



include/mesos/mesos.proto (line 1029)
<https://reviews.apache.org/r/45932/#comment191313>

    This comment doesn't seem to add anything?



include/mesos/mesos.proto (line 1032)
<https://reviews.apache.org/r/45932/#comment191315>

    Can we omit this? Why did you include it?



include/mesos/mesos.proto (lines 1035 - 1036)
<https://reviews.apache.org/r/45932/#comment191314>

    Avoid saying "running" here since there are more non-terminal states than just RUNNING:
    
    ```
    // Non-terminal tasks.
    ```



src/slave/slave.cpp (line 5169)
<https://reviews.apache.org/r/45932/#comment191316>

    let's say "non-terminal" instead of running, ideally slave.hpp doesn't say "running" either,
but let's leave it for now



src/slave/slave.cpp (lines 5171 - 5176)
<https://reviews.apache.org/r/45932/#comment191317>

    How about s/taskEntry/t/ ?



src/tests/oversubscription_tests.cpp (lines 230 - 231)
<https://reviews.apache.org/r/45932/#comment191318>

    How about s/task_label/key/ s/task_label_value/value/ ? Will it fit on one line then?
    
    We generally avoid "foo" and "bar" in favor of things like "key" and "value" to make the
test clearer, so please ignore the executor labels here.


- Ben Mahler


On April 8, 2016, 5:19 p.m., Zhitao Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45932/
> -----------------------------------------------------------
> 
> (Updated April 8, 2016, 5:19 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-5030
>     https://issues.apache.org/jira/browse/MESOS-5030
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add stripped TaskInfo's to ResourceUsage.Executor message.
> 
> 
> Diffs
> -----
> 
>   CHANGELOG 4553465cc3dc17956f168469d405f7a453d6359e 
>   include/mesos/mesos.proto e1fc02e05df531e29601c6764a5a48ba2b18569f 
>   include/mesos/v1/mesos.proto 35789e051608ea7f1be3ba5b63eaa1fc4e501c84 
>   src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3 
>   src/tests/oversubscription_tests.cpp 23671746da2ac505d75bc2bd59114697d9161d52 
> 
> Diff: https://reviews.apache.org/r/45932/diff/
> 
> 
> Testing
> -------
> 
> Added new test to verify ResourceUsage sees task labels.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>


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