----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58185/#review171140 -----------------------------------------------------------
Looks good! Mostly minor style nits/naming suggestions. Ccan you split this patch into two: - The first one would move the `log(...)` function into the common file and make the master/agent code use it. - This patch would be responsible for making the `Files` route handlers use the common logging function that we added in the earlier review? src/common/http.hpp Lines 227 (patched) <https://reviews.apache.org/r/58185/#comment244026> s/logHttpRequest/logRequest to make the function name protocol agnostic? src/files/files.cpp Lines 209 (patched) <https://reviews.apache.org/r/58185/#comment244032> We already do `using http = ...` in the file above. Can we remove the redundant `process::` prefix? src/files/files.cpp Lines 226 (patched) <https://reviews.apache.org/r/58185/#comment244028> Nit: Can we remove the redundant `this` and just use `download(...)` directly? src/files/files.cpp Lines 233 (patched) <https://reviews.apache.org/r/58185/#comment244029> Ditto as above src/files/files.cpp Lines 273 (patched) <https://reviews.apache.org/r/58185/#comment244030> Would fit on the line above? Ditto for the other occurences src/slave/slave.hpp Line 508 (original) <https://reviews.apache.org/r/58185/#comment244027> Can we still keep this comment in `src/common/http.cpp`? - Anand Mazumdar On April 4, 2017, 8:11 p.m., James Peach wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58185/ > ----------------------------------------------------------- > > (Updated April 4, 2017, 8:11 p.m.) > > > Review request for mesos and Anand Mazumdar. > > > Bugs: MESOS-7340 > https://issues.apache.org/jira/browse/MESOS-7340 > > > Repository: mesos > > > Description > ------- > > Consolidate the master and agent HTTP request log helper functions > into common code. Add HTTP access logging to `FilesProcess` so that the > `/files` endpoint logs consistently. > > > Diffs > ----- > > src/common/http.hpp b6e61f7f7f8ebcf5b25a37684cae06cb96188478 > src/common/http.cpp 89133e0175ffb792ae95705315844a32550470a1 > src/files/files.cpp f066146b7cbff35c452717d179b79039bc603cc8 > src/master/http.cpp 6cf9d350446d1b2d4a6e67d552217daff32657ff > src/master/master.hpp 1b077424373d6e195e4ab29e150dedbc3f3f95ab > src/master/master.cpp 6a6a570e52d21bfb2443f981e3d7faf8c36f74bc > src/slave/http.cpp e253ce9749fc8a03c21dac1ba0e6efe09311414b > src/slave/slave.hpp e4f46d42b3c0d0f09cff2d896abf6b84aed6c396 > src/slave/slave.cpp 65e4a67888fe908e5b2f6ca2ecc9e3a5b9958b2e > > > Diff: https://reviews.apache.org/r/58185/diff/1/ > > > Testing > ------- > > Make check (Fedora 25). Manual inspection of log output. > > > Thanks, > > James Peach > >
