> 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; > > }; > > > > ``` > > Alexander Rukletsov wrote: > 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? > > Michael Park wrote: > @Vinod: Just for clarification, > > causing both old and new allocators to coexist at the same time and > talking to the same master. > They do coexist at the same time, but they don't talk to the same master. > > My suggestion here is to contain each of the runs in their own lexical > scope. Here's what `FrameworkReregistersFirst` could look like with this > approach: > ``` > TYPED_TEST(MasterAllocatorTest, FrameworkReregistersFirst) > { > // Components that last both runs. > MockExecutor exec(DEFAULT_EXECUTOR_ID); > > StandaloneMasterDetector slaveDetector; > > slave::Flags flags = this->CreateSlaveFlags(); > flags.resources = Some("cpus:2;mem:1024"); > > Try<PID<Slave> > slave = this->StartSlave(&exec, &slaveDetector, flags); > ASSERT_SOME(slave); > > MockScheduler sched; > StandaloneMasterDetector schedulerDetector; > TestingMesosSchedulerDriver driver(&sched, &schedulerDetector); > > driver.start(); > > /* run1 */ { > TestAllocator allocator1 = TestAllocator::create<TypeParam>(); > > Try<PID<Master>> master1 = this->StartMaster(&allocator1); > ASSERT_SOME(master1); > > schedulerDetector.appoint(master1.get()); > > slaveDetector.appoint(master1.get()); > > ... > > this->ShutdownMasters(); > } > > /* run2 */ { > TestAllocator allocator2 = TestAllocator::create<TypeParam>(); > > Try<PID<Master>> master2 = this->StartMaster(&allocator2); > ASSERT_SOME(master2); > > schedulerDetector.appoint(master2.get()); > > slaveDetector.appoint(master2.get()); > > ... > > this->ShutdownMasters(); > } > > // Shut everything down. > driver.stop(); > driver.join(); > > this->Shutdown(); > } > ``` > > I think the common components, and each of the runs are clearly captured > in this approach, the lifetimes of the objects are well contained within the > runs and we could use `allocator` and `master` in both scopes as well if we > felt like it. > > The `TestAllocator::create` above is suggested in > https://reviews.apache.org/r/31265. > If we find `TestAllocator::create<TypeParam>()` to be too verbose, we > could also provide a `createTestAllocator` alias in `MasterAllocatorTest`: > > ``` > template <typename T> > class MasterAllocatorTest : public MesosTest > { > > ... > > TestAllocator createTestAllocator() const { > return TestAllocator::create<T>(); > } > > ... > }; > ``` > > Michael Park wrote: > Oops, messed up the formatting in reply to @Vinod. Trying again. > > @Vinod: Just for clarification, > > causing both old and new allocators to coexist at the same time and > talking to the same master. > > They do coexist at the same time, but they don't talk to the same master.
Following offline chats with @Vinod and @MPark I have decided to remove Allocator instance from the test fixture and create it explicitly in each test to increase awareness about lifetime dependencies. Also I will use explicit scopes to ensure allocator instance is destroyed when the appropriate master is gone. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31263/#review78710 ----------------------------------------------------------- On April 15, 2015, 2:18 p.m., Alexander Rukletsov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31263/ > ----------------------------------------------------------- > > (Updated April 15, 2015, 2:18 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 42e42ac425a448fcc5e93db1cef1112cbf5e67c4 > 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 > >
