----------------------------------------------------------- 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 > >
