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

Thanks for working on this. Looks good, just one major issue around handling 
`TaskHealthStatus` messages + minor suggested cleanups.

Would love to take a more detailed look after the cleanups.

include/mesos/v1/mesos.proto (lines 1796 - 1813)

    hmmm .. Did you test if the health check workflow works?
    IIUC, the `mesos-health-check` binary sends a `TaskHealthStatus` message 
back to the executor and that message is not of type `v1::TaskHealthStatus`. If 
we try to deserialize, it should fail at that point. 
    For now, it seems to me that the best course of action is to preserve/keep 
using the unversioned health check binary/message. In future, we might want to 
either modify the existing `mesos-health-check` binary to emit 
`v1::TaskHealthStatus` messages in addition to the unversioned ones or create a 
new binary for versioned health checks. I would recommend filing a JIRA and a 
TODO in the code mentioning this. Makes sense?

src/launcher/http_command_executor.cpp (line 29)

    Newline before.

src/launcher/http_command_executor.cpp (line 70)

    Move this before `std::vector`

src/launcher/http_command_executor.cpp (lines 72 - 76)

    Can we do a sweep and eliminate all existing occurences of types that still 
use the v1 namespace in this .cpp file even after the `using` declaration e.g., 
`v1::FrameworkId` etc. 
    Also, can you add more using directives like `TaskState/TaskID`?
    I am assuming after this cleanup the `.cpp` file would be free of any 
`v1::` directives.

src/launcher/http_command_executor.cpp (lines 75 - 76)

    Move these  before `v1::executor::Call` i.e. above L72.

src/launcher/http_command_executor.cpp (line 77)

    Nit: Can we also include all the things from `process` namespace that we 
use to be consistent like `Future/Clock/Owned` etc?

src/launcher/http_command_executor.cpp (line 99)

    Why did you move this? Let's have the ordering of member variables the same 
as command executor.
    Also thanks for resolving the ambiguity name in the name of `override` 
variable. I would love it that you make a similar change to the command 
executor code too.

src/launcher/http_command_executor.cpp (line 109)

    Let's have the ordering of member variables the same as command executor.

src/launcher/http_command_executor.cpp (line 112)

    Let's use the C++11 way of doing this:
    virtual ~HttpCommandExecutor() = default;

src/launcher/http_command_executor.cpp (line 113)

    Let's scope all the functions after this to the `protected` namespace. 
    I know that you had an initial look into the example code that has them in 
the `public` namespace. But, most of them are generally meant to be used as 
simple walkthrough code-samples.

src/launcher/http_command_executor.cpp (line 130)

    Nit: Newline before.

src/launcher/http_command_executor.cpp (line 158)

    Can we have the default value as `None()` here?
    Would avoid passing `None()` explicitly for cases where you don't have the 
`message` argument.

src/launcher/http_command_executor.cpp (line 165)

    Nit: newline before.

src/launcher/http_command_executor.cpp (line 168)

    Newline before.

src/launcher/http_command_executor.cpp (line 171)

    Newline before.

src/launcher/http_command_executor.cpp (line 199)

    Newline before. We typically have a new-line after a statement having 
multiple lines.

src/launcher/http_command_executor.cpp (line 205)

    We typically follow a procedural programming style. However, we also hate 
nested code as it's hard to read if it spans multiple lines. Let's try to 
create a function called `launch` here. Also, let's create relevant helper 
functions for all the switch cases except `ACKNOWDLEGED`/`SUBSCRIBED` since 
they seem trivial with 2-3 lines of code.
    Hence, the new code would look something like this:
    switch(case) {
      case LAUNCH:
      etc etc.

src/launcher/http_command_executor.cpp (line 210)

    Let's not try to capture this by reference. It hardly buys us anything here 
and is disallowed in our C++ style guide.

src/launcher/http_command_executor.cpp (line 230)

    New line here. I am assuming you won't need it when you move this to an 
helper function.

src/launcher/http_command_executor.cpp (line 237)

    New line here. I am assuming you won't need it when you move this to an 
helper function.

src/launcher/http_command_executor.cpp (line 791)

    Why was this moved from being over `overrride`?

src/launcher/http_command_executor.cpp (lines 806 - 807)

    Did you think naming it as `task` might result in ambiguity?

src/launcher/http_command_executor.cpp (line 810)

    Why the new line?

src/launcher/http_command_executor.cpp (line 901)

    AlexR recently did a sweep across the code to replace such calls. Would be 
good to not introduce new ones.

- Anand Mazumdar

On March 6, 2016, 9:08 a.m., Qian Zhang wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44424/
> -----------------------------------------------------------
> (Updated March 6, 2016, 9:08 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
> -----
>   include/mesos/v1/mesos.proto 31960a52061f70d80528fb8326522ae1d6f75b2c 
>   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