Re: Review Request 48014: Added the route handler for v1 agent API.

[Benjamin Hindman]

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


Fix it, then Ship it!





src/slave/http.cpp (line 265)
<https://reviews.apache.org/r/48014/#comment200462>

    Unused variable, but I'm guessing it's used in a subsequent patch. One 
thought here, to avoid the declared but undefined `response` variable here and 
in the master/http.cpp, would be to define a helper function that you used 
instead? Not a blocker IMHO, but wanted to suggest something like:
    
    ```
    // Short comment about what this is doing here ...
    auto serializer = [request](const v1::agent::Response& response) -> 
Response {
      ContentType responseContentType;
      if (request.acceptsMediaType(APPLICATION_JSON)) {
        responseContentType = ContentType::JSON;
      } else if (request.acceptsMediaType(APPLICATION_PROTOBUF)) {
        responseContentType = ContentType::PROTOBUF;
      } else {
        return NotAcceptable(
            string("Expecting 'Accept' to allow ") +
            "'" + APPLICATION_PROTOBUF + "' or '" + APPLICATION_JSON + "'");
      }
    
      // TODO(vinod): Support JSONP requests?
      return OK(serialize(responseContentType, response),
                stringify(responseContentType));
    };
    ```
    
    Then in the code do things like:
    
    ```
    switch (call.type()) {
      ...
      case v1::agent::Call::GET_FLAGS:
        return serializer(evolve<v1::master::Response::GET_FLAGS>(_flags()));
      ...
      case v1::agent::Call::SOMETHING_ASYNC:
        return functionReturningFuture()
          .then([](const v1::agent::Response& response) {
            return serializer(response);
          });
          
      case v1::agent::Call::SOMETHING_ASYNC:
        return functionReturningFuture()
          .then(serializer); // This might even work?
    }
    ```



src/slave/http.cpp (line 275)
<https://reviews.apache.org/r/48014/#comment200460>

    Unnecessary (maybe fixed in subsequent patch?).


- Benjamin Hindman


On May 29, 2016, 7:36 p.m., Vinod Kone wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48014/
> -----------------------------------------------------------
> 
> (Updated May 29, 2016, 7:36 p.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
> -------
> 
> This is just the basic structure for handling the calls. Actual calls
> will be implemented later.
> 
> 
> Diffs
> -----
> 
>   src/slave/http.cpp 64a99d539a0957b92d42c25c8dda298199383018 
>   src/slave/slave.hpp 78a6ecfe29881f1723d7d603a99962afdd5a81a8 
>   src/slave/slave.cpp 97d15da2a20c74d3a502abac68cc7a695e06a189 
>   src/slave/validation.hpp 070a77115f0d6f22615fc5bd42537e6d1b86f0bf 
>   src/slave/validation.cpp dccc788d7ec782b08abe4b58d8b92031939fa2d7 
> 
> Diff: https://reviews.apache.org/r/48014/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>