Return-Path: X-Original-To: apmail-mesos-dev-archive@www.apache.org Delivered-To: apmail-mesos-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id EFD69173C7 for ; Fri, 17 Apr 2015 20:42:24 +0000 (UTC) Received: (qmail 51754 invoked by uid 500); 17 Apr 2015 20:42:24 -0000 Delivered-To: apmail-mesos-dev-archive@mesos.apache.org Received: (qmail 51686 invoked by uid 500); 17 Apr 2015 20:42:24 -0000 Mailing-List: contact dev-help@mesos.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@mesos.apache.org Delivered-To: mailing list dev@mesos.apache.org Received: (qmail 51601 invoked by uid 99); 17 Apr 2015 20:42:24 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 17 Apr 2015 20:42:24 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 2ECE71DB4A5; Fri, 17 Apr 2015 20:42:26 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============2583311587896339832==" MIME-Version: 1.0 Subject: Re: Review Request 31263: Refactored TestAllocator and allocator text fixture. From: "Michael Park" To: "Niklas Nielsen" , "Michael Park" , "Kapil Arya" Cc: "Vinod Kone" , "mesos" , "Alexander Rukletsov" Date: Fri, 17 Apr 2015 20:42:26 -0000 Message-ID: <20150417204226.1422.45367@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org/ Auto-Submitted: auto-generated Sender: "Michael Park" X-ReviewGroup: mesos X-ReviewRequest-URL: https://reviews.apache.org/r/31263/ X-Sender: "Michael Park" References: <20150402195815.16791.98873@reviews.apache.org> In-Reply-To: <20150402195815.16791.98873@reviews.apache.org> Reply-To: "Michael Park" X-ReviewRequest-Repository: mesos --===============2583311587896339832== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit > On April 2, 2015, 7:58 p.m., Vinod Kone wrote: > > src/tests/master_allocator_tests.cpp, lines 96-104 > > > > > > 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 > > class MasterAllocatorTest : public MesosTest > > { > > protected: > > MasterAllocatorTest() : allocator(Null) {} > > > > Try create() > > { > > // We don't support multiple allocators because... > > if (allocator != NULL) { > > return Error("Multiple active allocators are not supported"); > > } > > > > allocator = new TestAllocator(process::Owned(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 > slave = this->StartSlave(&exec, &slaveDetector, flags); > ASSERT_SOME(slave); > > MockScheduler sched; > StandaloneMasterDetector schedulerDetector; > TestingMesosSchedulerDriver driver(&sched, &schedulerDetector); > > driver.start(); > > /* run1 */ { > TestAllocator allocator1 = TestAllocator::create(); > > Try> master1 = this->StartMaster(&allocator1); > ASSERT_SOME(master1); > > schedulerDetector.appoint(master1.get()); > > slaveDetector.appoint(master1.get()); > > ... > > this->ShutdownMasters(); > } > > /* run2 */ { > TestAllocator allocator2 = TestAllocator::create(); > > Try> 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()` to be too verbose, we could also provide a `createTestAllocator` alias in `MasterAllocatorTest`: > > ``` > template > class MasterAllocatorTest : public MesosTest > { > > ... > > TestAllocator createTestAllocator() const { > return TestAllocator::create(); > } > > ... > }; > ``` 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. - Michael ----------------------------------------------------------- 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 > > --===============2583311587896339832==--