Re: Review Request 54220: Made the agent's `api/v1` handler handle request streaming.

[Vinod Kone <vi...@gmail.com>]

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




src/slave/http.cpp (line 340)
<https://reviews.apache.org/r/54220/#comment228127>

    s/,/or/
    s/,/or/
    
    I think comma here might be confusing "content type should be 'a,b,c' or 'd'"



src/slave/http.cpp (lines 365 - 378)
<https://reviews.apache.org/r/54220/#comment228134>

    not sure why this block has anything to do with `requestStreaming`? why can't it just be singly nested 'if else' statements?



src/slave/http.cpp (lines 411 - 416)
<https://reviews.apache.org/r/54220/#comment228135>

    i think this looks better if it's indented as
    
    ```
    return _api(call.get(),
                std::move(reader),
                contentType,
                acceptType.get(),
                principal);
    ```
    
    since `return _api(` is a short string.



src/slave/http.cpp (lines 420 - 432)
<https://reviews.apache.org/r/54220/#comment228137>

    I would put this in an else block to make the semantics clear.
    
    We typically do 
    ```
    if (error) {
      // handle error.
      return;
    }
    
    if (some other error) {
      // handle error.
      return;
    }
    
    // handle normal case
    
    ```
    when handling error cases.
    
    but i don't think that style makes sense when both blocks are equally valid. in that case it should be `if else` or `switch`.



src/slave/http.cpp (line 443)
<https://reviews.apache.org/r/54220/#comment228139>

    s/does/has/



src/slave/http.cpp (lines 450 - 451)
<https://reviews.apache.org/r/54220/#comment228140>

    put "&&" on the above lien and indent "call.type()" with "!request"


- Vinod Kone


On Nov. 30, 2016, 7:18 p.m., Anand Mazumdar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54220/
> -----------------------------------------------------------
> 
> (Updated Nov. 30, 2016, 7:18 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6472
>     https://issues.apache.org/jira/browse/MESOS-6472
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This lays the ground work for adding the attach container input
> call later.
> 
> 
> Diffs
> -----
> 
>   src/common/http.hpp 378208be58d0cd141ff60a6d25a173f2793870dc 
>   src/common/http.cpp 42af33094150001fa6741effb6b0aee1cde3c109 
>   src/slave/http.cpp 87189dd6e2e099cb74faabd3ad26aaca11e5cef2 
>   src/slave/slave.hpp 05865a09724f7a46e9c06b02e59779513fc532aa 
> 
> Diff: https://reviews.apache.org/r/54220/diff/
> 
> 
> Testing
> -------
> 
> make check (tests would be added later in the chain)
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>