mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Isabel Jimenez" <cont...@isabeljimenez.com>
Subject Re: Review Request 36402: Adding 'Accept' header in request
Date Tue, 04 Aug 2015 23:36:03 GMT


> On Aug. 4, 2015, 7:25 p.m., Ben Mahler wrote:
> > Looking better! Can you please pull out the fixes to 'acceptsEncoding' into a separate
review? Also, would like to see tests for the cases that weren't handled correctly (e.g. "gzipp;q=1.0"
should not match "gzip"). Pulling the changes apart helps avoid extra round-trips on reviews
(see here: http://mesos.apache.org/documentation/latest/effective-code-reviewing/) :)

Just added the dependency patch in the description of this RR.


> On Aug. 4, 2015, 7:25 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/http.cpp, lines 158-160
> > <https://reviews.apache.org/r/36402/diff/6/?file=1028798#file1028798line158>
> >
> >     We don't need to use pairs anymore, right? Looks like we should just call tokenize
again on 'tokens[1]' (if present).

I don't think there's a real difference. Is this change really necessary?


> On Aug. 4, 2015, 7:25 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/http.cpp, lines 220-222
> > <https://reviews.apache.org/r/36402/diff/6/?file=1028798#file1028798line220>
> >
> >     Ditto here, can we tokenize 'tokens[1]' instead of using strings::pairs?

See comment above.


> On Aug. 4, 2015, 7:25 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/http.cpp, line 236
> > <https://reviews.apache.org/r/36402/diff/6/?file=1028798#file1028798line236>
> >
> >     newline here?

Added.


> On Aug. 4, 2015, 7:25 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/tests/http_tests.cpp, line 693
> > <https://reviews.apache.org/r/36402/diff/6/?file=1028799#file1028799line693>
> >
> >     Mind moving this above with the other http tests?

Moved.


> On Aug. 4, 2015, 7:25 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/tests/http_tests.cpp, lines 696-703
> > <https://reviews.apache.org/r/36402/diff/6/?file=1028799#file1028799line696>
> >
> >     We don't generally wrap things this far, can you just do the following?
> >     
> >     ```
> >      vector<string> wrongHeaders = {
> >          "test,q=0.0",
> >          ...
> >      };
> >     ```
> >     
> >     Ditto below.

Changed style.


> On Aug. 4, 2015, 7:25 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/tests/http_tests.cpp, lines 709-710
> > <https://reviews.apache.org/r/36402/diff/6/?file=1028799#file1028799line709>
> >
> >     Ditto here, can you just print 'accept'?

Printing the exact case where it failed may help later to debug in case of failure.


> On Aug. 4, 2015, 7:25 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/tests/http_tests.cpp, line 706
> > <https://reviews.apache.org/r/36402/diff/6/?file=1028799#file1028799line706>
> >
> >     'auto' here isn't buying us much over just 'const string&', we try to use
'auto' where it helps readability
> >     
> >     Also please use foreach for now

I don't see why it will not help with readability.


- Isabel


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


On Aug. 4, 2015, 11:05 p.m., Isabel Jimenez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36402/
> -----------------------------------------------------------
> 
> (Updated Aug. 4, 2015, 11:05 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone.
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> -------
> 
> Adding a method for Accept header in request + refactor of Accept-Encoding
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/http.hpp b8d9300 
>   3rdparty/libprocess/src/http.cpp 4dcbd74 
>   3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd5 
> 
> Diff: https://reviews.apache.org/r/36402/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message