mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ben Mahler" <benjamin.mah...@gmail.com>
Subject Re: Review Request 36217: Adding http validations to master call request validations
Date Tue, 07 Jul 2015 01:52:52 GMT

-----------------------------------------------------------
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
> 
>


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