mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marco Massenzio" <ma...@mesosphere.io>
Subject Re: Review Request 30612: Added /master/frameworks/{framework}/tasks/{task} endpoint.
Date Tue, 21 Apr 2015 00:51:42 GMT

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


The code looks fine, but I have some misgivings about the logic (see comments) when invalid
requests are sent.
If this has already been discussed and agreed upon, then I'm happy to go with the flow (just
noting here for the record, that it makes ours a really "unconventional" API)

I love the amount of testing!


src/master/http.cpp
<https://reviews.apache.org/r/30612/#comment131003>

    nit: s/requested/requester (or request)



src/master/http.cpp
<https://reviews.apache.org/r/30612/#comment131004>

    I don't like this at all.
    
    Invalid ID: 400 BAD REQUEST
    No task/framework for ID: 404 NOT FOUND
    
    There is a fundamental, semantic difference between there being no tasks (empty array)
for a valid ID, and you asking me for gibberish



src/master/http.cpp
<https://reviews.apache.org/r/30612/#comment131005>

    argubaly this should be DEBUG (but don't really feel strongly about this)



src/master/http.cpp
<https://reviews.apache.org/r/30612/#comment131006>

    can we have all routes patters as CONSTANTS defined in the class header? definitely easier
to debug etc.



src/master/http.cpp
<https://reviews.apache.org/r/30612/#comment131007>

    nit: s/and/an (**an** empty list)



src/master/http.cpp
<https://reviews.apache.org/r/30612/#comment131009>

    as mentioned, I'd much prefer
    ```
    return NotFound("No Framework found for ID: "
        + path["framework"])
    ```



src/master/http.cpp
<https://reviews.apache.org/r/30612/#comment131010>

    ditto


- Marco Massenzio


On April 20, 2015, 4:27 p.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30612/
> -----------------------------------------------------------
> 
> (Updated April 20, 2015, 4:27 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Marco Massenzio, Niklas Nielsen, and Till Toenshoff.
> 
> 
> Bugs: MESOS-2157
>     https://issues.apache.org/jira/browse/MESOS-2157
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Adds endpoints for paths /master/frameworks/{framework}/tasks/{task}.
> 
> Adds tests for the endpoints.
> 
> Works with [29883](https://reviews.apache.org/r/29883).
> 
> Builds on the abandoned patch 14286.
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp 00c22c43bd1f6cef7963b2ffa9c095c6cbd01cd3 
>   src/master/master.hpp c10e7c08c191acef9d31d98018a47a2a952a4dfc 
>   src/master/master.cpp e30b951eda2b3b0d5b2a80716f0b32c6bbe041bc 
>   src/tests/master_tests.cpp 32b1e9bb58d8046e5363fafe2ab8f662b6c9a666 
> 
> Diff: https://reviews.apache.org/r/30612/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>


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