You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Vinod Kone <vi...@gmail.com> on 2016/12/01 03:22:13 UTC
Re: Review Request 54221: Added the v1 `ATTACH_CONTAINER_INPUT` call
handler on the agent.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54221/#review157518
-----------------------------------------------------------
can you add a simple test for this change as discussed. comprehensive tests can come later.
src/slave/http.cpp (line 518)
<https://reviews.apache.org/r/54221/#comment228142>
why is `principal` not being passed?
also the signatures of the API handlers are a bit inconsistent now
```
# most of the handlers are
foo(call, principal, contentType)
attachContainerInput(contentType, acceptType, call, reader)
attachContainerOutput(call, contentType, acceptType, principal)
launchContainerOutput(call, contentType, acceptType, principal)
```
i propose we do
```
# NOTE: s/contentType/acceptType/ for consistency
foo(call, acceptType, principal)
attachContainerInput(call, reader, contentType, acceptType, principal)
attachContainerOutput(call, contentType, acceptType, principal)
launchContainerOutput(call, contentType, acceptType, principal)
```
i can do the change for `foo`. can you change `attachContainerInput` in this patch?
src/slave/http.cpp (line 2172)
<https://reviews.apache.org/r/54221/#comment228151>
s/reader/decoder/
we have been naming recordio::Reader as `decoder` when it conflicts with the naming of Pipe::Reder.
src/slave/http.cpp (line 2180)
<https://reviews.apache.org/r/54221/#comment228152>
if you change the above, this will be
s/reader_/reader/
src/slave/http.cpp (line 2190)
<https://reviews.apache.org/r/54221/#comment228160>
Can you expand on why you need to write the first record manually, for posterity?
src/slave/http.cpp (lines 2193 - 2194)
<https://reviews.apache.org/r/54221/#comment228161>
why do this here instead of inside the lambda?
src/slave/http.cpp (line 2230)
<https://reviews.apache.org/r/54221/#comment228163>
This is different from how we prolong the lifetime of `connection` in `attachContainerOutput`. can we make them consistent?
- Vinod Kone
On Nov. 30, 2016, 10:09 p.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54221/
> -----------------------------------------------------------
>
> (Updated Nov. 30, 2016, 10:09 p.m.)
>
>
> Review request for mesos, Kevin Klues and Vinod Kone.
>
>
> Bugs: MESOS-6472
> https://issues.apache.org/jira/browse/MESOS-6472
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added the v1 `ATTACH_CONTAINER_INPUT` call handler on the agent.
>
>
> Diffs
> -----
>
> src/slave/http.cpp 87189dd6e2e099cb74faabd3ad26aaca11e5cef2
> src/slave/slave.hpp 05865a09724f7a46e9c06b02e59779513fc532aa
>
> Diff: https://reviews.apache.org/r/54221/diff/
>
>
> Testing
> -------
>
> make check (tests would be added soon and can't be committed without them)
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 54221: Added the v1 `ATTACH_CONTAINER_INPUT` call
handler on the agent.
Posted by Vinod Kone <vi...@gmail.com>.
> On Dec. 1, 2016, 3:22 a.m., Vinod Kone wrote:
> > src/slave/http.cpp, line 2231
> > <https://reviews.apache.org/r/54221/diff/2/?file=1573319#file1573319line2231>
> >
> > This is different from how we prolong the lifetime of `connection` in `attachContainerOutput`. can we make them consistent?
>
> Anand Mazumdar wrote:
> hmm, the scenarios looked pretty different and hence I did not make them consistent. In the previous scenario, we had already received the `response` from the `Connection` object and were streaming it back to the client. Hence, it was fine to just capture it in `transform()` to prolong the life time. However, here, the switch board would only _start_ streaming the response after it has received the streaming request fully.
>
> Vinod Kone wrote:
> I see the difference between the 2 scenarios, but I don't completely get why `connection` cannot be captured in the `transform` lambda here too? Are you saying that doesn't work?
>
> Anand Mazumdar wrote:
> Yes, it won't work in this case. `transform()` would complete as soon as we have completed streaming the `Request` to the switch board but before waiting for the response. Hence, we need this alternate way to prolong its lifetime.
aha. makes sense. thanks.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54221/#review157518
-----------------------------------------------------------
On Dec. 1, 2016, 5:43 a.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54221/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2016, 5:43 a.m.)
>
>
> Review request for mesos, Kevin Klues and Vinod Kone.
>
>
> Bugs: MESOS-6472
> https://issues.apache.org/jira/browse/MESOS-6472
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added the v1 `ATTACH_CONTAINER_INPUT` call handler on the agent.
>
>
> Diffs
> -----
>
> src/slave/http.cpp 87189dd6e2e099cb74faabd3ad26aaca11e5cef2
> src/slave/slave.hpp 05865a09724f7a46e9c06b02e59779513fc532aa
>
> Diff: https://reviews.apache.org/r/54221/diff/
>
>
> Testing
> -------
>
> make check (tests would be added soon and can't be committed without them)
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 54221: Added the v1 `ATTACH_CONTAINER_INPUT` call
handler on the agent.
Posted by Vinod Kone <vi...@gmail.com>.
> On Dec. 1, 2016, 3:22 a.m., Vinod Kone wrote:
> > src/slave/http.cpp, lines 2194-2195
> > <https://reviews.apache.org/r/54221/diff/2/?file=1573319#file1573319line2194>
> >
> > why do this here instead of inside the lambda?
>
> Anand Mazumdar wrote:
> C++11 does not support moving objects when capturing them in a lambda. So, it was done outside the lambda.
I see. Can you add a comment saying so for posterity?
> On Dec. 1, 2016, 3:22 a.m., Vinod Kone wrote:
> > src/slave/http.cpp, line 2231
> > <https://reviews.apache.org/r/54221/diff/2/?file=1573319#file1573319line2231>
> >
> > This is different from how we prolong the lifetime of `connection` in `attachContainerOutput`. can we make them consistent?
>
> Anand Mazumdar wrote:
> hmm, the scenarios looked pretty different and hence I did not make them consistent. In the previous scenario, we had already received the `response` from the `Connection` object and were streaming it back to the client. Hence, it was fine to just capture it in `transform()` to prolong the life time. However, here, the switch board would only _start_ streaming the response after it has received the streaming request fully.
I see the difference between the 2 scenarios, but I don't completely get why `connection` cannot be captured in the `transform` lambda here too? Are you saying that doesn't work?
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54221/#review157518
-----------------------------------------------------------
On Dec. 1, 2016, 3:59 a.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54221/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2016, 3:59 a.m.)
>
>
> Review request for mesos, Kevin Klues and Vinod Kone.
>
>
> Bugs: MESOS-6472
> https://issues.apache.org/jira/browse/MESOS-6472
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added the v1 `ATTACH_CONTAINER_INPUT` call handler on the agent.
>
>
> Diffs
> -----
>
> src/slave/http.cpp 87189dd6e2e099cb74faabd3ad26aaca11e5cef2
> src/slave/slave.hpp 05865a09724f7a46e9c06b02e59779513fc532aa
>
> Diff: https://reviews.apache.org/r/54221/diff/
>
>
> Testing
> -------
>
> make check (tests would be added soon and can't be committed without them)
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 54221: Added the v1 `ATTACH_CONTAINER_INPUT` call
handler on the agent.
Posted by Anand Mazumdar <an...@apache.org>.
> On Dec. 1, 2016, 3:22 a.m., Vinod Kone wrote:
> > src/slave/http.cpp, line 2231
> > <https://reviews.apache.org/r/54221/diff/2/?file=1573319#file1573319line2231>
> >
> > This is different from how we prolong the lifetime of `connection` in `attachContainerOutput`. can we make them consistent?
>
> Anand Mazumdar wrote:
> hmm, the scenarios looked pretty different and hence I did not make them consistent. In the previous scenario, we had already received the `response` from the `Connection` object and were streaming it back to the client. Hence, it was fine to just capture it in `transform()` to prolong the life time. However, here, the switch board would only _start_ streaming the response after it has received the streaming request fully.
>
> Vinod Kone wrote:
> I see the difference between the 2 scenarios, but I don't completely get why `connection` cannot be captured in the `transform` lambda here too? Are you saying that doesn't work?
Yes, it won't work in this case. `transform()` would complete as soon as we have completed streaming the `Request` to the switch board but before waiting for the response. Hence, we need this alternate way to prolong its lifetime.
- Anand
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54221/#review157518
-----------------------------------------------------------
On Dec. 1, 2016, 3:59 a.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54221/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2016, 3:59 a.m.)
>
>
> Review request for mesos, Kevin Klues and Vinod Kone.
>
>
> Bugs: MESOS-6472
> https://issues.apache.org/jira/browse/MESOS-6472
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added the v1 `ATTACH_CONTAINER_INPUT` call handler on the agent.
>
>
> Diffs
> -----
>
> src/slave/http.cpp 87189dd6e2e099cb74faabd3ad26aaca11e5cef2
> src/slave/slave.hpp 05865a09724f7a46e9c06b02e59779513fc532aa
>
> Diff: https://reviews.apache.org/r/54221/diff/
>
>
> Testing
> -------
>
> make check (tests would be added soon and can't be committed without them)
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 54221: Added the v1 `ATTACH_CONTAINER_INPUT` call
handler on the agent.
Posted by Anand Mazumdar <an...@apache.org>.
> On Dec. 1, 2016, 3:22 a.m., Vinod Kone wrote:
> > can you add a simple test for this change as discussed. comprehensive tests can come later.
yep, on it :-)
> On Dec. 1, 2016, 3:22 a.m., Vinod Kone wrote:
> > src/slave/http.cpp, line 2231
> > <https://reviews.apache.org/r/54221/diff/2/?file=1573319#file1573319line2231>
> >
> > This is different from how we prolong the lifetime of `connection` in `attachContainerOutput`. can we make them consistent?
hmm, the scenarios looked pretty different and hence I did not make them consistent. In the previous scenario, we had already received the `response` from the `Connection` object and were streaming it back to the client. Hence, it was fine to just capture it in `transform()` to prolong the life time. However, here, the switch board would only _start_ streaming the response after it has received the streaming request fully.
> On Dec. 1, 2016, 3:22 a.m., Vinod Kone wrote:
> > src/slave/http.cpp, lines 2194-2195
> > <https://reviews.apache.org/r/54221/diff/2/?file=1573319#file1573319line2194>
> >
> > why do this here instead of inside the lambda?
C++11 does not support moving objects when capturing them in a lambda. So, it was done outside the lambda.
> On Dec. 1, 2016, 3:22 a.m., Vinod Kone wrote:
> > src/slave/http.cpp, line 519
> > <https://reviews.apache.org/r/54221/diff/2/?file=1573319#file1573319line519>
> >
> > why is `principal` not being passed?
> >
> > also the signatures of the API handlers are a bit inconsistent now
> >
> > ```
> > # most of the handlers are
> > foo(call, principal, contentType)
> >
> > attachContainerInput(contentType, acceptType, call, reader)
> >
> > attachContainerOutput(call, contentType, acceptType, principal)
> >
> > launchContainerOutput(call, contentType, acceptType, principal)
> >
> > ```
> >
> > i propose we do
> > ```
> > # NOTE: s/contentType/acceptType/ for consistency
> > foo(call, acceptType, principal)
> >
> > attachContainerInput(call, reader, contentType, acceptType, principal)
> >
> > attachContainerOutput(call, contentType, acceptType, principal)
> >
> > launchContainerOutput(call, contentType, acceptType, principal)
> >
> > ```
> >
> > i can do the change for `foo`. can you change `attachContainerInput` in this patch?
+1 to the proposal except don't pass `principal` when its not needed. Some calls might not be interested in AuthZ at all.
I did not pass `principal` since we did not have AuthZ handling for this handler. Would add it since we know that it's already being worked on.
- Anand
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54221/#review157518
-----------------------------------------------------------
On Dec. 1, 2016, 3:59 a.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54221/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2016, 3:59 a.m.)
>
>
> Review request for mesos, Kevin Klues and Vinod Kone.
>
>
> Bugs: MESOS-6472
> https://issues.apache.org/jira/browse/MESOS-6472
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added the v1 `ATTACH_CONTAINER_INPUT` call handler on the agent.
>
>
> Diffs
> -----
>
> src/slave/http.cpp 87189dd6e2e099cb74faabd3ad26aaca11e5cef2
> src/slave/slave.hpp 05865a09724f7a46e9c06b02e59779513fc532aa
>
> Diff: https://reviews.apache.org/r/54221/diff/
>
>
> Testing
> -------
>
> make check (tests would be added soon and can't be committed without them)
>
>
> Thanks,
>
> Anand Mazumdar
>
>