----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26486/#review57522 -----------------------------------------------------------
Looking so much cleaner Tim! Thanks for taking it on to improve the original implementation. src/slave/containerizer/composing.cpp <https://reviews.apache.org/r/26486/#comment98256> { on newline please. src/slave/containerizer/composing.cpp <https://reviews.apache.org/r/26486/#comment98254> Can you add a comment here that your assumption is that even if we've forwarded the destroy to the containerizer we will in fact eventually get the 'launch' response and won't clean up 'containers_' until then. src/slave/containerizer/composing.cpp <https://reviews.apache.org/r/26486/#comment98245> I tend to swap these since I don'k like the idea of deleting something that is still in the container! ;-) src/slave/containerizer/composing.cpp <https://reviews.apache.org/r/26486/#comment98253> Could you make this a ternary if to clean it up? src/slave/containerizer/composing.cpp <https://reviews.apache.org/r/26486/#comment98246> s/ is//g src/slave/containerizer/composing.cpp <https://reviews.apache.org/r/26486/#comment98248> Can we comment on why calling through to destroy is "okay" even if that containerizer is not going to launch the container? src/slave/containerizer/composing.cpp <https://reviews.apache.org/r/26486/#comment98247> Great comment, and here's a wording/language suggestion: Record the fact that this container was asked to be destroyed so that we won't try and launch this container using any other containerizers in the event the current containerizer has decided it can't launch the container. src/slave/containerizer/composing.cpp <https://reviews.apache.org/r/26486/#comment98255> What if destroy is called twice? I think this breaks your invariant? src/tests/composing_containerizer_tests.cpp <https://reviews.apache.org/r/26486/#comment98252> EXPECT_PENDING? src/tests/composing_containerizer_tests.cpp <https://reviews.apache.org/r/26486/#comment98251> Cool, let's test this part too! I think all you will need is the following above: EXPECT_CALL(*mockContainerizer2, launch(_, _, _, _, _, _, _, _)) .Times(0); And then this at the bottom after AWAIT_READY(destroy); launchPromise.set(false); AWAIT_READY_EQ(false, launch); - Benjamin Hindman On Oct. 20, 2014, 10:08 p.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26486/ > ----------------------------------------------------------- > > (Updated Oct. 20, 2014, 10:08 p.m.) > > > Review request for mesos and Benjamin Hindman. > > > Bugs: MESOS-1884 and MESOS-1915 > https://issues.apache.org/jira/browse/MESOS-1884 > https://issues.apache.org/jira/browse/MESOS-1915 > > > Repository: mesos-git > > > Description > ------- > > Review: https://reviews.apache.org/r/26486 > > > Diffs > ----- > > src/Makefile.am c44a9ad47d6e1262949b9049f4ae25b049440d99 > src/slave/containerizer/composing.cpp > 9022700b628d9746a6a8a17c9fbf1b1988da6fca > src/tests/composing_containerizer_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/26486/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Timothy Chen > >
