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