> 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.
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. - Ben ----------------------------------------------------------- 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 > >
