mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anand Mazumdar <mazumdar.an...@gmail.com>
Subject Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.
Date Tue, 08 Mar 2016 00:08:51 GMT

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



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)
<https://reviews.apache.org/r/44424/#comment184380>

    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)
<https://reviews.apache.org/r/44424/#comment184328>

    Newline before.



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

    Move this before `std::vector`



src/launcher/http_command_executor.cpp (lines 72 - 76)
<https://reviews.apache.org/r/44424/#comment184378>

    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)
<https://reviews.apache.org/r/44424/#comment184331>

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



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

    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)
<https://reviews.apache.org/r/44424/#comment184333>

    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)
<https://reviews.apache.org/r/44424/#comment184334>

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



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

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



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

    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)
<https://reviews.apache.org/r/44424/#comment184337>

    Nit: Newline before.



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

    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)
<https://reviews.apache.org/r/44424/#comment184343>

    Nit: newline before.



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

    Newline before.



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

    Newline before.



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

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



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

    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:
    
    ```cpp
    switch(case) {
      case LAUNCH:
        launch(...);
        break;
      etc etc.
    }
    ```



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

    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)
<https://reviews.apache.org/r/44424/#comment184350>

    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)
<https://reviews.apache.org/r/44424/#comment184351>

    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)
<https://reviews.apache.org/r/44424/#comment184386>

    Why was this moved from being over `overrride`?



src/launcher/http_command_executor.cpp (lines 806 - 807)
<https://reviews.apache.org/r/44424/#comment184383>

    s/unAckedUpdates/updates
    s/unAckedTask/task
    
    Did you think naming it as `task` might result in ambiguity?



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

    Why the new line?



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

    s/EXIT(1)/EXIT(EXIT_FAILURE)
    
    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
> 
>


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