You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Anand Mazumdar <an...@apache.org> on 2016/11/30 19:18:35 UTC

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

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

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

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

Posted by 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
> 
>

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

Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54220/#review157538
-----------------------------------------------------------


Ship it!




Ship It!

- Vinod Kone


On Dec. 1, 2016, 3:13 a.m., Anand Mazumdar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54220/
> -----------------------------------------------------------
> 
> (Updated Dec. 1, 2016, 3:13 a.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
> 
>

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

Posted by Anand Mazumdar <an...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54220/
-----------------------------------------------------------

(Updated Dec. 1, 2016, 3:13 a.m.)


Review request for mesos and Vinod Kone.


Changes
-------

Review comments.


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 (updated)
-----

  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