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



This looks pretty good. One more round of minor cleanups and we would get there.


src/launcher/http_command_executor.cpp (line 51)
<https://reviews.apache.org/r/44424/#comment190093>

    Can you reorder alphabetically?



src/launcher/http_command_executor.cpp (line 74)
<https://reviews.apache.org/r/44424/#comment190010>

    Nit: Can you move this above `std::string`?



src/launcher/http_command_executor.cpp (lines 80 - 88)
<https://reviews.apache.org/r/44424/#comment190012>

    Can you reorder these alphabetically?



src/launcher/http_command_executor.cpp (line 89)
<https://reviews.apache.org/r/44424/#comment190000>

    Nit: Newline before



src/launcher/http_command_executor.cpp (line 158)
<https://reviews.apache.org/r/44424/#comment190004>

    s/launchTask/launch
    
    This is more in sync with what we have been using as the name for the 
launch function for the v1 API.



src/launcher/http_command_executor.cpp (line 163)
<https://reviews.apache.org/r/44424/#comment190005>

    s/killTask/kill



src/launcher/http_command_executor.cpp (lines 195 - 199)
<https://reviews.apache.org/r/44424/#comment190025>

    How about:
    
    ```cpp
    // TODO(qianzhang): Currently, the `mesos-health-check` binary can only 
send unversioned `TaskHealthStatus` messages. This needs to be revisited as 
part of MESOS-5103.
    ```



src/launcher/http_command_executor.cpp (line 216)
<https://reviews.apache.org/r/44424/#comment190026>

    We use 4 line indent for function arguments. Can you fix this?



src/launcher/http_command_executor.cpp (lines 217 - 218)
<https://reviews.apache.org/r/44424/#comment190027>

    I would fix the corresponding command executor to not take boolean 
arguments by reference as part of  https://reviews.apache.org/r/45707/ 
    
    Can you fix it here?



src/launcher/http_command_executor.cpp (line 227)
<https://reviews.apache.org/r/44424/#comment190095>

    Indent?



src/launcher/http_command_executor.cpp (line 260)
<https://reviews.apache.org/r/44424/#comment190030>

    It looks a bit weird that the other function `kill` has an argument of just 
`TaskID` and this has an argument of `Event`.
    
    Can we modify the argument to be just `const Task& _task`



src/launcher/http_command_executor.cpp (line 289)
<https://reviews.apache.org/r/44424/#comment190099>

    2 space indent for continuation statements.



src/launcher/http_command_executor.cpp (line 297)
<https://reviews.apache.org/r/44424/#comment190100>

    2 space indent for continuation statements.



src/launcher/http_command_executor.cpp (line 423)
<https://reviews.apache.org/r/44424/#comment190104>

    Why new line? Also, should fit in one line, no?



src/launcher/http_command_executor.cpp (lines 464 - 466)
<https://reviews.apache.org/r/44424/#comment190075>

    Why change this? What's wrong with the previous version spanning 2 lines?



src/launcher/http_command_executor.cpp (line 592)
<https://reviews.apache.org/r/44424/#comment190073>

    The default argument for the last argument is anyways `None()`. Why have 
this redundant bit here? Also, would fit in one line neverthless.



src/launcher/http_command_executor.cpp (line 594)
<https://reviews.apache.org/r/44424/#comment190087>

    Why not do this after L273 i.e. as soon as you have verified if a task has 
not yet been launched and right before you capture the `TaskID` i.e.
    
    ```cpp
    // Capture the task.
    task = _task;
    ```
    
    We can then directly use it instead of using `_task`. Sound reasonable?



src/launcher/http_command_executor.cpp (lines 652 - 655)
<https://reviews.apache.org/r/44424/#comment190070>

    Should fit in one line, no?
    
    Also if we have `healthy` as a default argument, we can get rid of the 
explicit `None()` too.



src/launcher/http_command_executor.cpp (line 813)
<https://reviews.apache.org/r/44424/#comment190069>

    Indent should be 4 spaces.



src/launcher/http_command_executor.cpp (line 815)
<https://reviews.apache.org/r/44424/#comment190068>

    hmm ... Why isn't this argument default initialized to `None()` too?



src/launcher/http_command_executor.cpp (lines 1003 - 1011)
<https://reviews.apache.org/r/44424/#comment190052>

    2 space indent here.


- Anand Mazumdar


On April 4, 2016, 9:11 a.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44424/
> -----------------------------------------------------------
> 
> (Updated April 4, 2016, 9:11 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-3558
>     https://issues.apache.org/jira/browse/MESOS-3558
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Updated http_command_executor.cpp to use v1 API.
> 
> 
> Diffs
> -----
> 
>   src/launcher/http_command_executor.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44424/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Reply via email to