This is an automatically generated e-mail. To reply, visit:

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

src/launcher/http_command_executor.cpp (line 51)

    Can you reorder alphabetically?

src/launcher/http_command_executor.cpp (line 74)

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

src/launcher/http_command_executor.cpp (lines 80 - 88)

    Can you reorder these alphabetically?

src/launcher/http_command_executor.cpp (line 89)

    Nit: Newline before

src/launcher/http_command_executor.cpp (line 158)

    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)


src/launcher/http_command_executor.cpp (lines 195 - 199)

    How about:
    // 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)

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

src/launcher/http_command_executor.cpp (lines 217 - 218)

    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)


src/launcher/http_command_executor.cpp (line 260)

    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)

    2 space indent for continuation statements.

src/launcher/http_command_executor.cpp (line 297)

    2 space indent for continuation statements.

src/launcher/http_command_executor.cpp (line 423)

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

src/launcher/http_command_executor.cpp (lines 464 - 466)

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

src/launcher/http_command_executor.cpp (line 592)

    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)

    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.
    // 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)

    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)

    Indent should be 4 spaces.

src/launcher/http_command_executor.cpp (line 815)

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

src/launcher/http_command_executor.cpp (lines 1003 - 1011)

    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