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


Thanks Isabel!

Left some comments per our chat. Also, can we inline this into the /call 
handler? After adjusting this, it's going to become less generally useful, and 
the 'Accept' logic could even become subsumed by a more intelligent route().


src/common/http_validation.cpp (lines 39 - 64)
<https://reviews.apache.org/r/36217/#comment143749>

    Couple things per our conversation:
    
    (1) If we don't have the 'Accept' header, the spec says to assume they 
accept everything (i.e. `"*/*"`), so let's reply with json in this case to make 
it easier for folks. Alternatively, if you want to be more cautious, we could 
reject these but I'm guessing this is going to surprise many people given that 
no `"Accept"` and `"Accept = */*"` are semantically the same.
    
    (2) The way you're checking 'Accept' is not complete (look at the RFC and 
Request::accepts, which implements the 'Accept-Encoding' logic, (we should 
rename this to Request::acceptsEncoding). At the very least, let's add a TODO 
for now that this is incomplete.
    
    (3) Why is the 'Connection' header required? For HTTP 1.1, there is only a 
'close' value, and if ommitted, we can assume a persistent connection. So we 
don't need to reject requests without this. This is all the more reason to have 
some types around these common headers (e.g. Connection enum), but let's punt 
on that. :)


- Ben Mahler


On July 6, 2015, 11:31 p.m., Isabel Jimenez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36217/
> -----------------------------------------------------------
> 
> (Updated July 6, 2015, 11:31 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-2860
>     https://issues.apache.org/jira/browse/MESOS-2860
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> -------
> 
> Adding helper validations for http requests
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am addb63f 
>   src/common/http_validation.hpp PRE-CREATION 
>   src/common/http_validation.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/36217/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Reply via email to