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 2017/01/25 19:48:51 UTC
Review Request 55940: Added support for the new streaming
request/response headers.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55940/
-----------------------------------------------------------
Review request for mesos and Vinod Kone.
Bugs: MESOS-6936
https://issues.apache.org/jira/browse/MESOS-6936
Repository: mesos
Description
-------
Modified the Agent API/Switchboard handlers to support the
"Message-Accept"/"Message-Content-Type" headers for request/response
streaming. Also, adds general validations to ensure that non-streaming
requests/responses don't have these headers set.
Diffs
-----
src/slave/containerizer/mesos/io/switchboard.cpp 1b8f490872b8a13dc7bc64883ed28080752f82b6
src/slave/http.cpp 85990aee42195f8d2e0affa06c0bb5724f229247
src/slave/slave.hpp 0dadbe50be15f89b791da55fa10f1b434693ee0f
Diff: https://reviews.apache.org/r/55940/diff/
Testing
-------
make check
Thanks,
Anand Mazumdar
Re: Review Request 55940: Added support for the new streaming
request/response headers.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55940/#review163343
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On Jan. 26, 2017, 5:03 a.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55940/
> -----------------------------------------------------------
>
> (Updated Jan. 26, 2017, 5:03 a.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-6936
> https://issues.apache.org/jira/browse/MESOS-6936
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Modified the Agent API/Switchboard handlers to support the
> "Message-Accept"/"Message-Content-Type" headers for request/response
> streaming. Also, adds general validations to ensure that non-streaming
> requests/responses don't have these headers set.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/io/switchboard.cpp 1b8f490872b8a13dc7bc64883ed28080752f82b6
> src/slave/http.cpp 85990aee42195f8d2e0affa06c0bb5724f229247
> src/slave/slave.hpp 0dadbe50be15f89b791da55fa10f1b434693ee0f
>
> Diff: https://reviews.apache.org/r/55940/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 55940: Added support for the new streaming
request/response headers.
Posted by Anand Mazumdar <an...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55940/
-----------------------------------------------------------
(Updated Jan. 26, 2017, 5:03 a.m.)
Review request for mesos and Vinod Kone.
Changes
-------
Rebase.
Bugs: MESOS-6936
https://issues.apache.org/jira/browse/MESOS-6936
Repository: mesos
Description
-------
Modified the Agent API/Switchboard handlers to support the
"Message-Accept"/"Message-Content-Type" headers for request/response
streaming. Also, adds general validations to ensure that non-streaming
requests/responses don't have these headers set.
Diffs (updated)
-----
src/slave/containerizer/mesos/io/switchboard.cpp 1b8f490872b8a13dc7bc64883ed28080752f82b6
src/slave/http.cpp 85990aee42195f8d2e0affa06c0bb5724f229247
src/slave/slave.hpp 0dadbe50be15f89b791da55fa10f1b434693ee0f
Diff: https://reviews.apache.org/r/55940/diff/
Testing
-------
make check
Thanks,
Anand Mazumdar
Re: Review Request 55940: Added support for the new streaming
request/response headers.
Posted by Anand Mazumdar <an...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55940/
-----------------------------------------------------------
(Updated Jan. 26, 2017, 4:51 a.m.)
Review request for mesos and Vinod Kone.
Changes
-------
Review comments
Bugs: MESOS-6936
https://issues.apache.org/jira/browse/MESOS-6936
Repository: mesos
Description
-------
Modified the Agent API/Switchboard handlers to support the
"Message-Accept"/"Message-Content-Type" headers for request/response
streaming. Also, adds general validations to ensure that non-streaming
requests/responses don't have these headers set.
Diffs (updated)
-----
src/slave/containerizer/mesos/io/switchboard.cpp 1b8f490872b8a13dc7bc64883ed28080752f82b6
src/slave/http.cpp 85990aee42195f8d2e0affa06c0bb5724f229247
src/slave/slave.hpp 0dadbe50be15f89b791da55fa10f1b434693ee0f
Diff: https://reviews.apache.org/r/55940/diff/
Testing
-------
make check
Thanks,
Anand Mazumdar
Re: Review Request 55940: Added support for the new streaming
request/response headers.
Posted by Anand Mazumdar <an...@apache.org>.
> On Jan. 26, 2017, 12:58 a.m., Vinod Kone wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp, lines 1299-1322
> > <https://reviews.apache.org/r/55940/diff/1/?file=1615314#file1615314line1299>
> >
> > Not yours, but I'm wondering if these should be CHECKs because the agent API handler already validates these.
Good catch! Added another review in the chain to make the as explicit assertions.
> On Jan. 26, 2017, 12:58 a.m., Vinod Kone wrote:
> > src/slave/http.cpp, line 374
> > <https://reviews.apache.org/r/55940/diff/1/?file=1615315#file1615315line374>
> >
> > do you have to cast here?
Yeah, we need to add an assignment operator for `Option<T>` from a `T&&`.
- Anand
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55940/#review163057
-----------------------------------------------------------
On Jan. 25, 2017, 7:48 p.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55940/
> -----------------------------------------------------------
>
> (Updated Jan. 25, 2017, 7:48 p.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-6936
> https://issues.apache.org/jira/browse/MESOS-6936
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Modified the Agent API/Switchboard handlers to support the
> "Message-Accept"/"Message-Content-Type" headers for request/response
> streaming. Also, adds general validations to ensure that non-streaming
> requests/responses don't have these headers set.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/io/switchboard.cpp 1b8f490872b8a13dc7bc64883ed28080752f82b6
> src/slave/http.cpp 85990aee42195f8d2e0affa06c0bb5724f229247
> src/slave/slave.hpp 0dadbe50be15f89b791da55fa10f1b434693ee0f
>
> Diff: https://reviews.apache.org/r/55940/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 55940: Added support for the new streaming
request/response headers.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55940/#review163057
-----------------------------------------------------------
src/slave/containerizer/mesos/io/switchboard.cpp (lines 1299 - 1320)
<https://reviews.apache.org/r/55940/#comment234496>
Not yours, but I'm wondering if these should be CHECKs because the agent API handler already validates these.
src/slave/containerizer/mesos/io/switchboard.cpp (line 1668)
<https://reviews.apache.org/r/55940/#comment234498>
move this to #1672
src/slave/http.cpp (line 372)
<https://reviews.apache.org/r/55940/#comment234486>
do you have to cast here?
src/slave/http.cpp (line 412)
<https://reviews.apache.org/r/55940/#comment234493>
Can you add a comment here
// For backwards compatibility, if a client does not specify an 'Accept' header,
// 'Content-Type' of the response is set to 'application/json' both for
// non-streaming and streaming responses.
//
// TODO(Anand): In v2 API, the default 'Content-Type' for streaming responses should
// be 'application/recordio'.
src/slave/http.cpp (line 440)
<https://reviews.apache.org/r/55940/#comment234488>
should this be `NotAcceptable` instead?
- Vinod Kone
On Jan. 25, 2017, 7:48 p.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55940/
> -----------------------------------------------------------
>
> (Updated Jan. 25, 2017, 7:48 p.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-6936
> https://issues.apache.org/jira/browse/MESOS-6936
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Modified the Agent API/Switchboard handlers to support the
> "Message-Accept"/"Message-Content-Type" headers for request/response
> streaming. Also, adds general validations to ensure that non-streaming
> requests/responses don't have these headers set.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/io/switchboard.cpp 1b8f490872b8a13dc7bc64883ed28080752f82b6
> src/slave/http.cpp 85990aee42195f8d2e0affa06c0bb5724f229247
> src/slave/slave.hpp 0dadbe50be15f89b791da55fa10f1b434693ee0f
>
> Diff: https://reviews.apache.org/r/55940/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Anand Mazumdar
>
>