mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Hindman <b...@berkeley.edu>
Subject Re: Review Request 48012: Implemented v1::master::Call::GET_FLAGS.
Date Sun, 29 May 2016 20:00:01 GMT

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


Fix it, then Ship it!





src/internal/evolve.hpp (line 106)
<https://reviews.apache.org/r/48012/#comment200449>

    I think this could use a little bit more of a comment, especially since this is not an
evolution of a protobuf, it's an evolution of JSON! ;-) What actually is an "old style JSON
message"? Trying to be helpful here, how about something like:
    
    Before the v1 API we had REST endpoints that returned JSON. The JSON was not specified
in any formal way, i.e., there were not protobufs which captured the structure. As part of
the v1 API we introduced the `Call/Response` protobufs (see documentation at ...). This `evolve`
variant transforms a JSON object that would have been returned from a particular REST endpoint
to a `Response` protobuf suitable for returning from the new `Call` endpoints.



src/internal/evolve.cpp (line 394)
<https://reviews.apache.org/r/48012/#comment200450>

    I think `evolveFlags` is fine, but an alterantive here would be to define a templated
function that let you skip the `switch` statement you have below completely. Then if the implementation
you're looking for doesn't exist the code won't compile (or at least won't link). For example:
    
    // Declaration of helper functions for evolving JSON
    // objects used in REST endpoints pre v1 API.
    template <v1::master::Response::Type T>
    v1::master::Response evolve(const JSON::Object& object);
    
    template <>
    v1::master::Response evolve<v1::master::Response::GET_FLAGS>(const JSON::Object&
object)
    {
      ...;
    }
    
    Any definitions missing should fail at link time.



src/internal/evolve.cpp (line 400)
<https://reviews.apache.org/r/48012/#comment200451>

    = on this line?



src/internal/evolve.cpp (line 423)
<https://reviews.apache.org/r/48012/#comment200452>

    Great factor out here Vinod!



src/master/http.cpp (line 28)
<https://reviews.apache.org/r/48012/#comment200453>

    Move below the other multi-depth level includes?



src/master/http.cpp (line 1101)
<https://reviews.apache.org/r/48012/#comment200456>

    It doesn't look like `__flags` needs to be asynchronous, any reason to not do it that
way?
    
    ```
    return OK(__flags(), request.url.query.get("jsonp"));
    ```
    
    In fact, you could probably pull this all the way up in to `flags` and then make `_flags`
be the shared function.



src/master/http.cpp (line 1108)
<https://reviews.apache.org/r/48012/#comment200457>

    This function doesn't appear to need to be asynchronous (i.e., returning a future).



src/master/http.cpp (lines 1133 - 1136)
<https://reviews.apache.org/r/48012/#comment200458>

    Perhaps could just be:
    
    ```
    return evolve<v1::master::Response::GET_FLAGS>(_flags());
    ```
    
    And at that point you might as well just stick this up in the handler `api`.



src/master/http.cpp (line 1135)
<https://reviews.apache.org/r/48012/#comment200454>

    Given my comment above, this line would change only slightly to:
    
    return evolve<v1::master::Response::GET_FLAGS>(object);



src/master/validation.hpp (lines 27 - 28)
<https://reviews.apache.org/r/48012/#comment200459>

    We can use your `mesos/v1/master.hpp` now yeah? Here and everywhere else.


- Benjamin Hindman


On May 29, 2016, 1:19 a.m., Vinod Kone wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48012/
> -----------------------------------------------------------
> 
> (Updated May 29, 2016, 1:19 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Hindman, and Kevin Klues.
> 
> 
> Bugs: MESOS-2720
>     https://issues.apache.org/jira/browse/MESOS-2720
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The idea is to extract JSON::Object representation of flags into a
> function that can be used by both the old JSON REST API handler and the
> new v1 API handler.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am f1bd5e6e5515535c2aaf9ea8b205f4e8c7096ec5 
>   src/internal/evolve.hpp 4d1cb7f2a180f4f4a44491393393449d36ed616b 
>   src/internal/evolve.cpp e7afcdb629a509e8bb30ba29542f360066eb8ad4 
>   src/master/http.cpp bbbf0a00e486b96e036f59a3107789d0322bc6cd 
>   src/master/master.hpp 1a875c32eddfb6d884e3d0dda7f5716ee53966c3 
>   src/master/master.cpp 6442762c9fdfa368d5d9d7cd43b97f5addaf7f17 
>   src/master/validation.hpp 580f420a72f33290bcbe718bad839084d4209b8c 
>   src/master/validation.cpp 982b61da5fe34dbe46190477e75ee7a4e4cdce9c 
>   src/tests/api_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/48012/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>


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