> On April 2, 2015, 7:58 p.m., Vinod Kone wrote: > > src/tests/master_allocator_tests.cpp, lines 96-104 > > <https://reviews.apache.org/r/31263/diff/7/?file=912106#file912106line96> > > > > While the TearDown() avoids flakiness by ensuring that an allocator > > process doesn't exist after a test, it doesn't address cases where a master > > is restarted with a new allocator in the same test (e.g., > > FrameworkReregistersFirst and SlaveReregistersFirst) causing both old and > > new allocators to coexist at the same time and talking to the same master. > > > > I suggest to have methods on the test fixture that creates and deletes > > allocator explicitly (can be used by tests that restart master). The > > TearDown() will still delete any active allocator. For safety, ensure that > > only one allocator is active at any given time. > > > > Does that make sense? > > > > ``` > > template <typename T> > > class MasterAllocatorTest : public MesosTest > > { > > protected: > > MasterAllocatorTest() : allocator(Null) {} > > > > Try<TestAllocator> create() > > { > > // We don't support multiple allocators because... > > if (allocator != NULL) { > > return Error("Multiple active allocators are not supported"); > > } > > > > allocator = new TestAllocator(process::Owned<Allocator>(new T)); > > return *allocator; > > } > > > > void destroy() > > { > > delete allocator; > > allocator = NULL; > > } > > > > virtual void TearDown() > > { > > destroy(); > > MeosTest::TearDown(); > > } > > > > private: > > TestAllocator* allocator; > > }; > > > > ```
We used to have a lifecycle method but decided to remove it: be6246a11276074dfb60a892a890b80cb7cd37cd RR: https://reviews.apache.org/r/29927/ The two tests you point to are currently the only ones that create an additional allocator. They both create a new leader instance and appoint the slave instance to the new leader, and there is no possibility AFAIK to swap allocators in a leader instance. So yes, there are two active allocators in these tests, but they belong to different leaders. Current design ensures there is only one active allocator in the fixture, but allows to create more if needed (what those two tests do). The design you propose doesn't prevent us from creating one more allocator in the test body either. Thoughts? - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31263/#review78710 ----------------------------------------------------------- On April 1, 2015, 2:27 p.m., Alexander Rukletsov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31263/ > ----------------------------------------------------------- > > (Updated April 1, 2015, 2:27 p.m.) > > > Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. > > > Bugs: MESOS-2160 > https://issues.apache.org/jira/browse/MESOS-2160 > > > Repository: mesos > > > Description > ------- > > TestAllocator stores a pointer to a real allocator and takes over its > ownership. MasterAllocatorTest fixture stores the test allocator in turn. > > > Diffs > ----- > > src/tests/containerizer.cpp 26b87ac6b16dfeaf84888e80296ef540697bd775 > src/tests/master_allocator_tests.cpp > 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 > src/tests/mesos.hpp 0e98572a62ae05437bd2bc800c370ad1a0c43751 > src/tests/resource_offers_tests.cpp > 882a9ff4d09aace486182828bf43b643b0d0c519 > src/tests/slave_recovery_tests.cpp 87f4a6aab27d142fa8eb7a6571f684a6ce59687e > > Diff: https://reviews.apache.org/r/31263/diff/ > > > Testing > ------- > > make check (Mac OS 10.9.5, CentOS 7.0) > > > Thanks, > > Alexander Rukletsov > >
