> On Aug. 12, 2013, 6:53 p.m., Eric Biederman wrote: > > src/slave/isolator.hpp, line 111 > > <https://reviews.apache.org/r/13502/diff/1/?file=340123#file340123line111> > > > > Why are these methods virtual? If these are not meant to be overriden > > virtual seems wrong here. > > David Mackey wrote: > Given the presence of other virtual functions and Isolator serving as a > base class, a derived class may be destroyed through a pointer to the base > and if the base class destructor is not virtual then the derived class > destructor won't be called resulting in a potential resource leak. > > Ben Mahler wrote: > Would be good to add some NOTEs (similar to allocator.hpp) providing > guidance to extend from IsolatorProcess rather than Isolator, hence no need > for virtual methods on Isolator. You can leave the destructor virtual though > if you like.
It's better to remove all the virtuals and move the destructor to protected. Then no need for NOTEs. - David ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13502/#review25026 ----------------------------------------------------------- On Aug. 12, 2013, 6:42 p.m., Ian Downes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/13502/ > ----------------------------------------------------------- > > (Updated Aug. 12, 2013, 6:42 p.m.) > > > Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone. > > > Bugs: MESOS-579 > https://issues.apache.org/jira/browse/MESOS-579 > > > Repository: mesos-git > > > Description > ------- > > First part of a wrapper around the Isolator process. > > Does not yet return Futures instead of calling back to the slave. > > > Diffs > ----- > > src/local/local.cpp e4b5ec5 > src/slave/cgroups_isolator.hpp e86062e > src/slave/cgroups_isolator.cpp 3427c62 > src/slave/isolator.hpp fc13089 > src/slave/isolator.cpp c9643cf > src/slave/main.cpp 750a127 > src/slave/monitor.cpp 4f3c91f > src/slave/process_isolator.hpp 4ae093f > src/slave/process_isolator.cpp a80b047 > src/slave/slave.hpp 8ba605b > src/slave/slave.cpp 3b49118 > src/tests/cluster.hpp f743bb3 > src/tests/environment.cpp bf62bcf > src/tests/fault_tolerance_tests.cpp c8d88d5 > src/tests/gc_tests.cpp e404de3 > src/tests/isolator.hpp fe6b38d > src/tests/isolator_tests.cpp cd3b300 > src/tests/master_detector_tests.cpp 2d140ba > src/tests/master_tests.cpp 52f09d4 > src/tests/mesos.hpp 8fbd56c > src/tests/mesos.cpp 776cb0f > src/tests/monitor_tests.cpp 3142416 > src/tests/slave_recovery_tests.cpp bd755f6 > > Diff: https://reviews.apache.org/r/13502/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Ian Downes > >
