----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52330/#review150917 -----------------------------------------------------------
Fix it, then Ship it! src/slave/containerizer/composing.cpp (lines 339 - 342) <https://reviews.apache.org/r/52330/#comment218992> Hm.. this comment is not true anymore? I think you need an if here to check for the container having been removed by the destroy continuation. I think in this case we would just pass through the launch value. src/slave/containerizer/composing.cpp (lines 357 - 359) <https://reviews.apache.org/r/52330/#comment218995> Hm.. these two comments seem a bit redundant? src/slave/containerizer/composing.cpp (lines 363 - 377) <https://reviews.apache.org/r/52330/#comment218997> It looks like 3 of these comments say the same thing about none of the containerizers supporting the launch? src/slave/containerizer/composing.cpp (lines 513 - 516) <https://reviews.apache.org/r/52330/#comment219007> This isn't true anymore, seems like if it has been removed we just pass the launched value through and don't need to do anything else? src/slave/containerizer/composing.cpp (lines 518 - 544) <https://reviews.apache.org/r/52330/#comment219000> Would it be a bit simpler to do something like this? ``` if (launched) { if (container->state == LAUNCHING) { container->state = LAUNCHED; } } else { container->destroyed.set(false); containers_.erase(containerId); delete container; } return launched; ``` src/slave/containerizer/composing.cpp (lines 519 - 521) <https://reviews.apache.org/r/52330/#comment219003> The first sentence just seems to be a written form of the logic here? src/tests/containerizer/composing_containerizer_tests.cpp (line 249) <https://reviews.apache.org/r/52330/#comment219013> Should this be s/true/false/? This would be an implementation bug if launch isn't supported but destroy succeeds. src/tests/containerizer/composing_containerizer_tests.cpp (lines 251 - 252) <https://reviews.apache.org/r/52330/#comment219011> Whoops, s/true/false/ - Benjamin Mahler On Sept. 29, 2016, 4:12 a.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52330/ > ----------------------------------------------------------- > > (Updated Sept. 29, 2016, 4:12 a.m.) > > > Review request for mesos, Benjamin Mahler and Gilbert Song. > > > Bugs: MESOS-6260 > https://issues.apache.org/jira/browse/MESOS-6260 > > > Repository: mesos > > > Description > ------- > > Fixed the race between launch and destroy in composing containerizer. > > > Diffs > ----- > > src/slave/containerizer/composing.cpp > 1031326a81cbedb2659d8831a7f419a0944d40ad > src/tests/containerizer/composing_containerizer_tests.cpp > 631328ac54bd6c17abc82f2ea78dc2f5b5750253 > > Diff: https://reviews.apache.org/r/52330/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
