This is an automatically generated e-mail. To reply, visit:

This looks good, Isabel!
Just a few nits about error messages and my being obsessive about Error codes 
and Response types (HTTP has been around a while, and people have come to 
expect APIs to behave in a certain way).

src/master/http.cpp (line 297)

    from what little I've seen in the codebase, we use the leading underscore 
for "continuation" methods, is this such a method?
    If so, can we please make sure that we have the `validate()` method next to 

src/master/http.cpp (lines 308 - 309)

    I really really dislike hard-coded strings sprinkled all over the code (I'm 
sure you will need them elsewhere: if not now, soon :)
    Can we please collect them in some "well-known" place? 
    there is a constants.cpp file or we can have a "specialized" 
http_constants.cpp one (preferred).
    Bottom line: please avoid hard-coded constants (especially for 
commonly-used, well-known strings)

src/master/http.cpp (line 311)

    is the indent off?

src/master/http.cpp (line 313)

    bad error message - we should state that 
    Only `keep-alive` connection header allowed
    or something to that effect

src/master/http.cpp (line 315)

    this is surprising - please make sure to add a comment so that people won't 
expect `Some(response)` here...

src/master/http.cpp (line 331)

    this should be a
    405 Method Not Allowed
    we could also emit a better error message:
    return MethodNotAllowed("Only POST allowed, received " + request.method + " 

src/master/http.cpp (line 339)

    a better name would be contentType

src/master/http.cpp (line 342)

    capitalize the header name in the message too.

src/master/http.cpp (line 366)

    this should be a 415 (unsupported media type)
    see ref URL above
    also: emit the content type you received in the error message as a courtesy 
to your users:
    "Unsupported '" + contentType.get() + "' Content-Type; please only use 
application/json or application/x-protobuf"
    or something to that effect.

src/tests/call_tests.cpp (line 98)

    can we possibly check on just getting a "type" of response (and not on the 
actual message)?
    that would be a better check, so we won't fail tests just because we fix 
typos in messages :)

- Marco Massenzio

On June 30, 2015, 9:07 a.m., Isabel Jimenez wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36037/
> -----------------------------------------------------------
> (Updated June 30, 2015, 9:07 a.m.)
> Review request for mesos, Anand Mazumdar, Benjamin Hindman, Ben Mahler, Marco 
> Massenzio, and Vinod Kone.
> Bugs: MESOS-2860
>     https://issues.apache.org/jira/browse/MESOS-2860
> Repository: mesos-incubating
> Description
> -------
> Adding a call route with HTTP request header validations
> Diffs
> -----
>   src/master/http.cpp 3503833 
>   src/master/master.hpp af83d3e 
>   src/master/master.cpp 0782b54 
>   src/tests/call_tests.cpp PRE-CREATION 
>   src/tests/mesos.hpp 9157ac0 
> Diff: https://reviews.apache.org/r/36037/diff/
> Testing
> -------
> make check
> Thanks,
> Isabel Jimenez

Reply via email to