mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vinod Kone <vinodk...@gmail.com>
Subject Re: Review Request 54196: Added API handler for LAUNCH_NESTED_CONTAINER_SESSION.
Date Fri, 02 Dec 2016 04:42:44 GMT


> On Dec. 1, 2016, 7:49 p.m., Anand Mazumdar wrote:
> > src/slave/http.cpp, line 1982
> > <https://reviews.apache.org/r/54196/diff/2/?file=1573665#file1573665line1982>
> >
> >     Why not be explicit here and set it to `ContainerClass::DEFAULT` like we do
for the corresponding session one?

done.


> On Dec. 1, 2016, 7:49 p.m., Anand Mazumdar wrote:
> > src/slave/http.cpp, line 2197
> > <https://reviews.apache.org/r/54196/diff/2/?file=1573665#file1573665line2197>
> >
> >     Nit: s/goes way/is interrupted

i did s/goes way/breaks/


> On Dec. 1, 2016, 7:49 p.m., Anand Mazumdar wrote:
> > src/slave/http.cpp, lines 2234-2243
> > <https://reviews.apache.org/r/54196/diff/2/?file=1573665#file1573665line2234>
> >
> >     hmm, can we create a lambda destroy and just invoke it with a `message` argument
rather then duplicating it here and else where?

passing the message looks a bit ugly because we need to construct it with "+" and stringify().
so i opted to log the warning and call the lambda instead. let me know if that looks ok.


> On Dec. 1, 2016, 7:49 p.m., Anand Mazumdar wrote:
> > src/slave/http.cpp, line 2229
> > <https://reviews.apache.org/r/54196/diff/2/?file=1573665#file1573665line2229>
> >
> >     Can we resolve this TODO? This can happen often if the launched container as
part of session terminates. This would result in an EOF. 
> >     
> >     You might want to sync up with Kevin regarding other cases when this can happen.

as discussed offline, if the container terminates we would get a failure instead of EOF here?
but anyway, leveraged trick mentioned by mpark to combine the handlers.


> On Dec. 1, 2016, 7:49 p.m., Anand Mazumdar wrote:
> > src/tests/api_tests.cpp, line 3623
> > <https://reviews.apache.org/r/54196/diff/2/?file=1573668#file1573668line3623>
> >
> >     hmm, this test would break the moment Jie commits Kevin's changes to add support
to the containerizer for launching debug containers?

I reealized I used "TestContainerizer" and not "MesosContainerizer" here. So nothing broke
after Jie's changes. I would like to use TestContainerizer because it lets me test more functionality.


- Vinod


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


On Dec. 2, 2016, 4:42 a.m., Vinod Kone wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54196/
> -----------------------------------------------------------
> 
> (Updated Dec. 2, 2016, 4:42 a.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: MESOS-6471
>     https://issues.apache.org/jira/browse/MESOS-6471
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> In addition to launching the nested container the API handler
> ensures that the container is destroyed if the connection breaks.
> 
> 
> Diffs
> -----
> 
>   src/slave/http.cpp ace3575fd3a330f788e678283bc05e806cf9b264 
>   src/slave/slave.hpp cfec9dd5fa3550ba1192fc86b1ff0d73ee1671e0 
>   src/slave/validation.cpp 4005cfcd8bcdb923a73de471100c3567344234cf 
>   src/tests/api_tests.cpp ea6e037c18fd116eff473ee246faa504ec37b7da 
> 
> Diff: https://reviews.apache.org/r/54196/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> Added a basic test for now that tests the failure case. Will be adding more tests in
subsequent reviews.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message