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