> On April 4, 2016, 10:35 a.m., haosdent huang wrote: > > src/tests/cluster.cpp, line 437 > > <https://reviews.apache.org/r/45689/diff/1/?file=1324705#file1324705line437> > > > > how about > > > > ``` > > if (!containerizer) { > > return; > > } > > ``` > > Jiang Yan Xu wrote: > +1 this is better. > > James Peach wrote: > You can't early return because you have to call ``terminate()``. > > Jiang Yan Xu wrote: > Alright, overlooked it :) However it looks like the lines above this > suffer from the same problem: > > ``` > if (!cleanUpContainersInDestructor) { > return; > } > ``` > > In generl I think this is OK: > > > How about > > ``` > if (!containerizer || !cleanUpContainersInDestructor) { > terminate(); > return; > } > > ... > ``` > > James Peach wrote: > AFAICT skipping cleanup tasks when ``cleanUpContainersInDestructor`` is > ``false`` is by design.
Haosdent's suggestion works in this case. See explanation in my review posted below. - Joseph ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45689/#review126866 ----------------------------------------------------------- On April 4, 2016, 10:31 a.m., James Peach wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45689/ > ----------------------------------------------------------- > > (Updated April 4, 2016, 10:31 a.m.) > > > Review request for mesos, Joris Van Remoortere, Joseph Wu, and Jiang Yan Xu. > > > Repository: mesos > > > Description > ------- > > If Cluster::Slave::start() fails, make sure we don't crash in the > destructor. > > > Diffs > ----- > > src/tests/cluster.cpp 14d0d34fcb4c408ad996672394c39c84fd2be918 > > Diff: https://reviews.apache.org/r/45689/diff/ > > > Testing > ------- > > Make check. > > > Thanks, > > James Peach > >
