----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44424/#review127791 -----------------------------------------------------------
Looks good! Just one new major comment around moving the code to the `mesos::v1::internal` namespace that would allow you to deal with naming ambiguities better. I was playing around with your patch and the following github gist already has fixes for all the new issues that I have raised. https://gist.github.com/hatred/c0be071125f6715e9403f5673de1860f src/launcher/http_command_executor.cpp (lines 80 - 92) <https://reviews.apache.org/r/44424/#comment191210> Let's move this all out of the `mesos::internal` namespace. We typically have `using` declarations at the top of the cpp file. Also, I synced up with Vinod offline let's put all the cpp code in `mesos::v1::internal` namespace. Then, you won't have all the naming ambiguities that made you add the using declarations in `mesos::internal` namespace. src/launcher/http_command_executor.cpp (line 94) <https://reviews.apache.org/r/44424/#comment191211> Let's remove this out to the top of the file too. Also, let's break it into the following using declarations. ```cpp using process::Clock; using process::Future; using process::Owned; using process::Subprocess; using process::Timer; ``` src/launcher/http_command_executor.cpp (line 147) <https://reviews.apache.org/r/44424/#comment191178> Can we log the type of received event here similar to: https://github.com/apache/mesos/blob/master/src/examples/long_lived_executor.cpp#L98 src/launcher/http_command_executor.cpp (line 184) <https://reviews.apache.org/r/44424/#comment191179> Can we add a log for this similar to here: https://github.com/apache/mesos/blob/master/src/examples/long_lived_executor.cpp#L124 You would need to separate out `ERROR` in it's own `case` statement for this. src/launcher/http_command_executor.cpp (line 376) <https://reviews.apache.org/r/44424/#comment191215> We should be able to remove the `slave::` prefix since we already include it. src/launcher/http_command_executor.cpp (line 411) <https://reviews.apache.org/r/44424/#comment191216> Ditto src/launcher/http_command_executor.cpp (line 798) <https://reviews.apache.org/r/44424/#comment191212> We have been renaming this method as `update` in the recent reviews to be more in sync with the `v1` API. - Anand Mazumdar On April 7, 2016, 2:13 p.m., Qian Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44424/ > ----------------------------------------------------------- > > (Updated April 7, 2016, 2:13 p.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 > >
