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


Last high level comment - if Adam and/or Till don't agree, feel free to drop. 
Mostly cleanup suggestions:

 - Trying to reduce the nestedness of the json modeling
 - Striking a balance between reducing 

Adam, Till: Can you give it a final pass?


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

    I'd like to sharpen this a bit. How about something like:
    
    "Returns a JSON object describing the registered frameworks and tasks. The 
request can broaden or narrow the result by:"



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

    i.e. should be e.g. here, right? You are listing an exmaple.



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

    If you move this up above if (path.contains("framework")) and inverse the 
test,  so it becomes:
    
    if(!path.contains("framework")) {
      JSON::Array array = model(master->frameworks.registered);
    
      JSON::Object object;
      object.values["frameworks"] = array;
    
      return OK(object, request.query.get("jsonp"));
    }
    
    FrameworkID frameworkId;
    frameworkId.set_value(path["framework"]);
    
    Option<Framework*> framework =
        master->frameworks.registered.get(frameworkId);
    ...
    
    I think we can reduce the nestedness of the large block above and have the 
flow be:
    Broad -> Narrow



src/tests/master_tests.cpp
<https://reviews.apache.org/r/30612/#comment136078>

    s/tests/test/
    
    What does 'no data about frameworks' mean? Can you sharpen this a bit?



src/tests/master_tests.cpp
<https://reviews.apache.org/r/30612/#comment136079>

    s/one/frameworks/



src/tests/master_tests.cpp
<https://reviews.apache.org/r/30612/#comment136080>

    s/some/a non existing/?



src/tests/master_tests.cpp
<https://reviews.apache.org/r/30612/#comment136081>

    What are you checking the tasks for? Not being present?



src/tests/master_tests.cpp
<https://reviews.apache.org/r/30612/#comment136088>

    I don't think we should not have tests and asserts in SetUp(). Are all 
theses tasks and setup used in each test?



src/tests/master_tests.cpp
<https://reviews.apache.org/r/30612/#comment136087>

    Want to separate Frameworks_FrameworkIDEndpoint -> 
Frameworks_FrameworkID_Endpoint ?


- Niklas Nielsen


On May 19, 2015, 1:42 a.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30612/
> -----------------------------------------------------------
> 
> (Updated May 19, 2015, 1:42 a.m.)
> 
> 
> Review request for mesos, Adam B, Joerg Schad, 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 01dbdad9720a9cd30505683ae1117970cda918a0 
>   src/master/master.hpp da0a83510784f4f7dbd933e666ac12c04c413a62 
>   src/master/master.cpp eaea79df2c693d15087d70b3c9b988e57c894f8e 
>   src/tests/master_tests.cpp ba3858fb3026f2cc7af0523bd1a0c541eeafc34b 
> 
> Diff: https://reviews.apache.org/r/30612/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Reply via email to