> On Jan. 11, 2016, 12:12 a.m., Adam B wrote:
> > 3rdparty/libprocess/src/http.cpp, lines 1218-1221
> > <https://reviews.apache.org/r/41789/diff/11/?file=1189249#file1189249line1218>
> >
> >     Will you add `query` as a parameter only once you've moved `get()` over 
> > to this?
> >     
> >     Does anybody even set the `contentType` yet?
> 
> Yongqiao Wang wrote:
>     Yes, I planed to add query parametner once moved get() method. I will log 
> another JIRA to clear the get() and post() method.
>     
>     And I only found contentType is set in master_maintenance_tests.cpp:
>         // Initialize the default POST header.
>         headers["Content-Type"] = "application/json";

Sounds good. File the JIRA for get/post and then we can close this review issue.

Not sure if we need contentType (yet) if it's always "application/json". No 
harm in leaving it in, I guess. What do you think?


> On Jan. 11, 2016, 12:12 a.m., Adam B wrote:
> > 3rdparty/libprocess/src/http.cpp, line 1250
> > <https://reviews.apache.org/r/41789/diff/11/?file=1189249#file1189249line1250>
> >
> >     Why not continue to call it 'request'? `http::httpRequest()` seems 
> > redundant.
> 
> Yongqiao Wang wrote:
>     'request' is a key word (the Request variable name) in the post()/get() 
> method, for reducing the code modification, so I rename it to httpRequest.  
> Can we change it back to 'reqeust' in the JIRA to clear get() and post() 
> method?

I still hate seeing `http::httpRequest(...)` since it violates the Mesos style 
of avoiding redundancies. `http::request()` should be sufficiently scoped in 
callers to prevent confusing it with a `request` variable. If you're worried 
about the confusion within the function, then you can rename the parameter to 
`_request`, as recommended by 
http://mesos.apache.org/documentation/latest/c++-style-guide/
"We prepend constructor and function arguments with a leading underscore to 
avoid ambiguity and/or shadowing"

Callers could still use `request` for their local variables and 
`http::request()` for the function, as those can be easily scoped. Then you 
would only have to rename `request` to `_request` inside your new 
`http::request()` function, not across all callers.


- Adam


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


On Jan. 17, 2016, 10:48 p.m., Yongqiao Wang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41789/
> -----------------------------------------------------------
> 
> (Updated Jan. 17, 2016, 10:48 p.m.)
> 
> 
> Review request for mesos, Adam B, Joerg Schad, Joris Van Remoortere, Neil 
> Conway, and Qian Zhang.
> 
> 
> Bugs: MESOS-3763
>     https://issues.apache.org/jira/browse/MESOS-3763
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Expose the internal::http::request function in the header and not add an 
> additional method/overload for put function.
> 
> (TODO): Clean the other instances of post/get to use the http::request method.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/http.hpp 
> 1fe549e9c3c64b310048388d90ab04e5641e08a1 
>   3rdparty/libprocess/src/http.cpp 40fd87c4aa1417d4746a5e4268c30c0e55d0ec0e 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> c23d0bf1929686cfc42969f39ce046f4794539d4 
> 
> Diff: https://reviews.apache.org/r/41789/diff/
> 
> 
> Testing
> -------
> 
> Make & Make check successfully.
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>

Reply via email to