Re: Review Request 36037: Adding /call endpoint to Master

[Marco Massenzio <ma...@mesosphere.io>]

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


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)
<https://reviews.apache.org/r/36037/#comment142886>

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



src/master/http.cpp (lines 308 - 309)
<https://reviews.apache.org/r/36037/#comment142887>

    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)
<https://reviews.apache.org/r/36037/#comment142889>

    is the indent off?



src/master/http.cpp (line 313)
<https://reviews.apache.org/r/36037/#comment142890>

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



src/master/http.cpp (line 315)
<https://reviews.apache.org/r/36037/#comment142891>

    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)
<https://reviews.apache.org/r/36037/#comment142892>

    this should be a
    405 Method Not Allowed
    http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.6
    
    we could also emit a better error message:
    ```
    return MethodNotAllowed("Only POST allowed, received " + request.method + " instead");
    ```



src/master/http.cpp (line 339)
<https://reviews.apache.org/r/36037/#comment142893>

    a better name would be contentType



src/master/http.cpp (line 342)
<https://reviews.apache.org/r/36037/#comment142894>

    capitalize the header name in the message too.



src/master/http.cpp (line 366)
<https://reviews.apache.org/r/36037/#comment142895>

    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)
<https://reviews.apache.org/r/36037/#comment142904>

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