mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anand Mazumdar <an...@apache.org>
Subject Re: Review Request 53994: Added streaming support to `/api/v1` handler on the agent.
Date Mon, 28 Nov 2016 00:32:45 GMT

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



Looks good. Mostly comments around getting rid of the `_api()` handler/doing redundant input
handling in one place in the `api()` handler itself.

Here is the diff based on my comments later: https://gist.github.com/hatred/eaff00183041f00613bd6e83ed393813
(Feel free to apply it if you agree with the comments, there might be some style issues though)


src/slave/http.cpp (line 313)
<https://reviews.apache.org/r/53994/#comment227353>

    I would just use `contentType_` as this is too verbose.



src/slave/http.cpp (lines 324 - 325)
<https://reviews.apache.org/r/53994/#comment227354>

    4 space indent



src/slave/http.cpp (lines 330 - 345)
<https://reviews.apache.org/r/53994/#comment227355>

    hmm, I know we talked about this offline but I still don't see much utility in having
this method. We should be able to move this in the `api()` handler itself.



src/slave/http.cpp (line 334)
<https://reviews.apache.org/r/53994/#comment227356>

    We don't need to copy the entire request to get a non const reader.
    
    ```cpp
    Pipe::Reader reader = request.reader.get(); // Remove const.
    ```



src/slave/http.cpp (lines 348 - 351)
<https://reviews.apache.org/r/53994/#comment227357>

    We should do the `contentType`, `acceptType` parsing at one place in the `api()` handler
itself. It feels a bit weird that we need to get the `Content-Type` headers twice from the
`Request` object once in the `api()` handler and then again here.
    
    As per my earlier comment, if we get rid of the `_api()`, a possible signature for this
method can be:
    
    ```cpp
    Future<Response> Slave::Http::__api(
        ContentType contentType,
        ContentType acceptType,
        const string& body,
        const Option<string>& principal) const
    ```
    
    Note that this would mean that you can't pass the `Request` object as it is to the `attachContainerOutput`
handler later. You would need to create a new `Request` object. It is no longer acting like
a proxy handler anyways due to it invoking `transform()`.



src/slave/http.cpp (lines 356 - 373)
<https://reviews.apache.org/r/53994/#comment227358>

    This can be moved to the `api()` handler itself.


- Anand Mazumdar


On Nov. 26, 2016, 5:42 p.m., Vinod Kone wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53994/
> -----------------------------------------------------------
> 
> (Updated Nov. 26, 2016, 5:42 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6473
>     https://issues.apache.org/jira/browse/MESOS-6473
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Note that this change only updates the handler to correctly
> handle non-streaming calls given it receives `PIPE` requests.
> Handling streaming calls will come later.
> 
> 
> Diffs
> -----
> 
>   src/slave/http.cpp aa9f492fc74283b5a6f24a8b7189f1ecbe41488f 
>   src/slave/slave.hpp 0cc105418bf4e35b604ebe412048237814e495fb 
>   src/slave/slave.cpp 521f08d59cd78f9089d58cd3294f0ee4a099cd7f 
> 
> Diff: https://reviews.apache.org/r/53994/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>


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