You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by "Timothy St. Clair" <ts...@redhat.com> on 2014/01/09 22:59:58 UTC
Review Request 16764: Enable different http_parser versions.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16764/
-----------------------------------------------------------
Review request for mesos and Timothy St. Clair.
Repository: mesos-git
Description
-------
Update to enable support for different versions of http_parser then what is currently bundled in mesos.
Diffs
-----
3rdparty/libprocess/src/decoder.hpp 4c29229aabd11ac2a273238cb4f50b02b6d3d7a8
Diff: https://reviews.apache.org/r/16764/diff/
Testing
-------
Thanks,
Timothy St. Clair
Re: Review Request 16764: Enable different http_parser versions.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16764/#review31510
-----------------------------------------------------------
3rdparty/libprocess/src/decoder.hpp
<https://reviews.apache.org/r/16764/#comment60022>
A random thought during my commute home, do you need to check the result of http_parser_parse_url before you attempt to check what fields it has set? Or are you guaranteed that if it "failed" none of the below if checks will get invoked?
- Benjamin Hindman
On Jan. 9, 2014, 10:10 p.m., Timothy St. Clair wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16764/
> -----------------------------------------------------------
>
> (Updated Jan. 9, 2014, 10:10 p.m.)
>
>
> Review request for mesos and Timothy St. Clair.
>
>
> Bugs: MESOS-896
> https://issues.apache.org/jira/browse/MESOS-896
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Update to enable support for different versions of http_parser then what is currently bundled in mesos.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/decoder.hpp 4c29229aabd11ac2a273238cb4f50b02b6d3d7a8
>
> Diff: https://reviews.apache.org/r/16764/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy St. Clair
>
>
Re: Review Request 16764: Enable different http_parser versions.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Jan. 14, 2014, 6:44 a.m., Benjamin Hindman wrote:
> > I've taken the liberty of making the small cleanups mentioned in this review to keep things moving. Thanks for getting this to us Tim! Looking forward to the next ones!
Okay, this has been committed, see commit 72a1e19aa4b1419492e916dac999ae3127659b41 for formatting cleanup for future patches. Thanks!
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16764/#review31707
-----------------------------------------------------------
On Jan. 13, 2014, 10:13 p.m., Timothy St. Clair wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16764/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2014, 10:13 p.m.)
>
>
> Review request for mesos and Timothy St. Clair.
>
>
> Bugs: MESOS-896
> https://issues.apache.org/jira/browse/MESOS-896
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Update to enable support for different versions of http_parser then what is currently bundled in mesos.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/decoder.hpp 4c29229
>
> Diff: https://reviews.apache.org/r/16764/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy St. Clair
>
>
Re: Review Request 16764: Enable different http_parser versions.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16764/#review31707
-----------------------------------------------------------
Ship it!
I've taken the liberty of making the small cleanups mentioned in this review to keep things moving. Thanks for getting this to us Tim! Looking forward to the next ones!
3rdparty/libprocess/src/decoder.hpp
<https://reviews.apache.org/r/16764/#comment60446>
Still some whitespace that needs to get killed (here and elsewhere).
3rdparty/libprocess/src/decoder.hpp
<https://reviews.apache.org/r/16764/#comment60447>
Spaces around the '+' operator please.
3rdparty/libprocess/src/decoder.hpp
<https://reviews.apache.org/r/16764/#comment60448>
If this stuff wraps, we indent +4 after opening parens on a newline.
3rdparty/libprocess/src/decoder.hpp
<https://reviews.apache.org/r/16764/#comment60449>
This closing brace doesn't appear to be properly indented.
- Benjamin Hindman
On Jan. 13, 2014, 10:13 p.m., Timothy St. Clair wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16764/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2014, 10:13 p.m.)
>
>
> Review request for mesos and Timothy St. Clair.
>
>
> Bugs: MESOS-896
> https://issues.apache.org/jira/browse/MESOS-896
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Update to enable support for different versions of http_parser then what is currently bundled in mesos.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/decoder.hpp 4c29229
>
> Diff: https://reviews.apache.org/r/16764/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy St. Clair
>
>
Re: Review Request 16764: Enable different http_parser versions.
Posted by "Timothy St. Clair" <ts...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16764/
-----------------------------------------------------------
(Updated Jan. 13, 2014, 10:13 p.m.)
Review request for mesos and Timothy St. Clair.
Bugs: MESOS-896
https://issues.apache.org/jira/browse/MESOS-896
Repository: mesos-git
Description
-------
Update to enable support for different versions of http_parser then what is currently bundled in mesos.
Diffs (updated)
-----
3rdparty/libprocess/src/decoder.hpp 4c29229
Diff: https://reviews.apache.org/r/16764/diff/
Testing
-------
Thanks,
Timothy St. Clair
Re: Review Request 16764: Enable different http_parser versions.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16764/#review31485
-----------------------------------------------------------
Ship it!
Thanks Tim! Just style nits and then we'll get this committed.
3rdparty/libprocess/src/decoder.hpp
<https://reviews.apache.org/r/16764/#comment59981>
Please kill the whitespace here and everywhere else, thanks!
3rdparty/libprocess/src/decoder.hpp
<https://reviews.apache.org/r/16764/#comment59985>
We prefer non-abbreviated variable names. In some cases, like the signature of this function, we'll use single characters (but we really should have s/p/parser/ here). So:
s/ret/result/
See stout/os.hpp where we use result a lot.
3rdparty/libprocess/src/decoder.hpp
<https://reviews.apache.org/r/16764/#comment59983>
s/reworked/Reworked/
Also, what about "Reworked parsing for version >= 2.0."
3rdparty/libprocess/src/decoder.hpp
<https://reviews.apache.org/r/16764/#comment59984>
s/tUrlData/url/
3rdparty/libprocess/src/decoder.hpp
<https://reviews.apache.org/r/16764/#comment59982>
Please use '{' '}' for all if statements, even if they only have one line. Also, please put spaces around the '<<' operator (we prefer '1 + 2' rather than '1+2' for our operators).
3rdparty/libprocess/src/decoder.hpp
<https://reviews.apache.org/r/16764/#comment59986>
Please wrap this if greater than 80 characters:
decoder->request->path.append(
data + url.field_data[UF_PATH].off,
url.field_data[UF_PATH].len);
3rdparty/libprocess/src/decoder.hpp
<https://reviews.apache.org/r/16764/#comment59987>
Since this #endif is far enough away from the #if we tend to add a comment showing what this closes:
#endif // !(HTTP_PARSER_VERSION_MAJOR >=2)
- Benjamin Hindman
On Jan. 9, 2014, 10:10 p.m., Timothy St. Clair wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16764/
> -----------------------------------------------------------
>
> (Updated Jan. 9, 2014, 10:10 p.m.)
>
>
> Review request for mesos and Timothy St. Clair.
>
>
> Bugs: MESOS-896
> https://issues.apache.org/jira/browse/MESOS-896
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Update to enable support for different versions of http_parser then what is currently bundled in mesos.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/decoder.hpp 4c29229aabd11ac2a273238cb4f50b02b6d3d7a8
>
> Diff: https://reviews.apache.org/r/16764/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy St. Clair
>
>
Re: Review Request 16764: Enable different http_parser versions.
Posted by "Timothy St. Clair" <ts...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16764/
-----------------------------------------------------------
(Updated Jan. 9, 2014, 10:10 p.m.)
Review request for mesos and Timothy St. Clair.
Bugs: MESOS-896
https://issues.apache.org/jira/browse/MESOS-896
Repository: mesos-git
Description
-------
Update to enable support for different versions of http_parser then what is currently bundled in mesos.
Diffs
-----
3rdparty/libprocess/src/decoder.hpp 4c29229aabd11ac2a273238cb4f50b02b6d3d7a8
Diff: https://reviews.apache.org/r/16764/diff/
Testing
-------
Thanks,
Timothy St. Clair