You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2014/01/18 00:41:27 UTC
Review Request 17076: Refined http get/post interfaces to use Option.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17076/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Charlie Carson.
Repository: mesos-git
Description
-------
See summary.
Diffs
-----
3rdparty/libprocess/include/process/http.hpp a8b1636
3rdparty/libprocess/src/process.cpp 936f118
Diff: https://reviews.apache.org/r/17076/diff/
Testing
-------
will include test fixes in the following patches.
Thanks,
Jie Yu
Re: Review Request 17076: Refined http get/post interfaces to use Option.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Jan. 17, 2014, 11:52 p.m., Charlie Carson wrote:
> > 3rdparty/libprocess/include/process/http.hpp, line 503
> > <https://reviews.apache.org/r/17076/diff/1/?file=429763#file429763line503>
> >
> > this is probably a API consistency question and I'm the new guy - but to me, a post w/o either a body or a contentType is incorrect. The entire purpose of a POST is to upload a body and you need to know the content type to know what to do with the body when it arrives.
>
> Jie Yu wrote:
> ContentType is not mandatory as far as I know. The server can infer the ContentType.
>
> http://www.w3.org/Protocols/rfc2616/rfc2616-sec7.html#sec7.2.1
>
> One can choose to POST an empty request (no body). In that case, ContentType is not needed.
>
> Charlie Carson wrote:
> someone up here mention /quitquitquit being a good example of POST w/ no body, so that's definitely a good point. OTOH, the spec says "Any HTTP/1.1 message containing an entity-body SHOULD include a Content-Type header field". so that makes me want to do two overloads:
>
> Future<Response> post(const UPID& upid, const Option<string>& path)
> Future<Response> post(const UPID& upid, const Option<string>& path, const string& contentType, const string& body)
>
> but the second overload won't compile... the other option would be to have a little struct which represents a entity-body - ex:
> struct Content {
> string contentType;
> string body;
> }
>
> then you could have a single overload for post:
>
> Future<Response> post(const UPID& upid, const Option<string>& path, const Option<PostContent>& content)
>
> that's nice b/c it would position you for posting things like the contents of a file or a stream (like the HttpResponse does).
>
> That said, we are DEFINITELY into deep api / framework style at this point and I'll defer to you.
>
> Feel free to close the issue.
>
>
>
Given that the Content-Type isn't necessary I think moving it after the body makes sense (since it has a default of 'None()' and is one less parameter that would need to get passed). But given that 'path' could also be 'None()' and isn't after 'body' I don't think it would be bad to pull 'contentType' before 'body' to account for the intuitiveness of how it appears on the wire.
> On Jan. 17, 2014, 11:52 p.m., Charlie Carson wrote:
> > 3rdparty/libprocess/src/process.cpp, line 3627
> > <https://reviews.apache.org/r/17076/diff/1/?file=429764#file429764line3627>
> >
> > what is the rationale for reversing the order of body and contenttype? I would think we'd want them in order they appear on the wire in the request so that they are more intuitive.
Yup, let's swap it back.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17076/#review32226
-----------------------------------------------------------
On Jan. 17, 2014, 11:41 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17076/
> -----------------------------------------------------------
>
> (Updated Jan. 17, 2014, 11:41 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Charlie Carson.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp a8b1636
> 3rdparty/libprocess/src/process.cpp 936f118
>
> Diff: https://reviews.apache.org/r/17076/diff/
>
>
> Testing
> -------
>
> will include test fixes in the following patches.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 17076: Refined http get/post interfaces to use Option.
Posted by Jie Yu <yu...@gmail.com>.
> On Jan. 17, 2014, 11:52 p.m., Charlie Carson wrote:
> > 3rdparty/libprocess/include/process/http.hpp, line 503
> > <https://reviews.apache.org/r/17076/diff/1/?file=429763#file429763line503>
> >
> > this is probably a API consistency question and I'm the new guy - but to me, a post w/o either a body or a contentType is incorrect. The entire purpose of a POST is to upload a body and you need to know the content type to know what to do with the body when it arrives.
ContentType is not mandatory as far as I know. The server can infer the ContentType.
http://www.w3.org/Protocols/rfc2616/rfc2616-sec7.html#sec7.2.1
One can choose to POST an empty request (no body). In that case, ContentType is not needed.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17076/#review32226
-----------------------------------------------------------
On Jan. 17, 2014, 11:41 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17076/
> -----------------------------------------------------------
>
> (Updated Jan. 17, 2014, 11:41 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Charlie Carson.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp a8b1636
> 3rdparty/libprocess/src/process.cpp 936f118
>
> Diff: https://reviews.apache.org/r/17076/diff/
>
>
> Testing
> -------
>
> will include test fixes in the following patches.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 17076: Refined http get/post interfaces to use Option.
Posted by Charlie Carson <cc...@twitter.com>.
> On Jan. 17, 2014, 11:52 p.m., Charlie Carson wrote:
> > 3rdparty/libprocess/include/process/http.hpp, line 503
> > <https://reviews.apache.org/r/17076/diff/1/?file=429763#file429763line503>
> >
> > this is probably a API consistency question and I'm the new guy - but to me, a post w/o either a body or a contentType is incorrect. The entire purpose of a POST is to upload a body and you need to know the content type to know what to do with the body when it arrives.
>
> Jie Yu wrote:
> ContentType is not mandatory as far as I know. The server can infer the ContentType.
>
> http://www.w3.org/Protocols/rfc2616/rfc2616-sec7.html#sec7.2.1
>
> One can choose to POST an empty request (no body). In that case, ContentType is not needed.
someone up here mention /quitquitquit being a good example of POST w/ no body, so that's definitely a good point. OTOH, the spec says "Any HTTP/1.1 message containing an entity-body SHOULD include a Content-Type header field". so that makes me want to do two overloads:
Future<Response> post(const UPID& upid, const Option<string>& path)
Future<Response> post(const UPID& upid, const Option<string>& path, const string& contentType, const string& body)
but the second overload won't compile... the other option would be to have a little struct which represents a entity-body - ex:
struct Content {
string contentType;
string body;
}
then you could have a single overload for post:
Future<Response> post(const UPID& upid, const Option<string>& path, const Option<PostContent>& content)
that's nice b/c it would position you for posting things like the contents of a file or a stream (like the HttpResponse does).
That said, we are DEFINITELY into deep api / framework style at this point and I'll defer to you.
Feel free to close the issue.
- Charlie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17076/#review32226
-----------------------------------------------------------
On Jan. 17, 2014, 11:41 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17076/
> -----------------------------------------------------------
>
> (Updated Jan. 17, 2014, 11:41 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Charlie Carson.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp a8b1636
> 3rdparty/libprocess/src/process.cpp 936f118
>
> Diff: https://reviews.apache.org/r/17076/diff/
>
>
> Testing
> -------
>
> will include test fixes in the following patches.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 17076: Refined http get/post interfaces to use Option.
Posted by Charlie Carson <cc...@twitter.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17076/#review32226
-----------------------------------------------------------
3rdparty/libprocess/include/process/http.hpp
<https://reviews.apache.org/r/17076/#comment60955>
this is probably a API consistency question and I'm the new guy - but to me, a post w/o either a body or a contentType is incorrect. The entire purpose of a POST is to upload a body and you need to know the content type to know what to do with the body when it arrives.
3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/17076/#comment60957>
what is the rationale for reversing the order of body and contenttype? I would think we'd want them in order they appear on the wire in the request so that they are more intuitive.
- Charlie Carson
On Jan. 17, 2014, 11:41 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17076/
> -----------------------------------------------------------
>
> (Updated Jan. 17, 2014, 11:41 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Charlie Carson.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp a8b1636
> 3rdparty/libprocess/src/process.cpp 936f118
>
> Diff: https://reviews.apache.org/r/17076/diff/
>
>
> Testing
> -------
>
> will include test fixes in the following patches.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 17076: Refined http get/post interfaces to use Option.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17076/#review32247
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/17076/#comment60983>
Did we miss an os::close here?
- Ben Mahler
On Jan. 17, 2014, 11:41 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17076/
> -----------------------------------------------------------
>
> (Updated Jan. 17, 2014, 11:41 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Charlie Carson.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp a8b1636
> 3rdparty/libprocess/src/process.cpp 936f118
>
> Diff: https://reviews.apache.org/r/17076/diff/
>
>
> Testing
> -------
>
> will include test fixes in the following patches.
>
>
> Thanks,
>
> Jie Yu
>
>