You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Alexander Rukletsov <al...@mesosphere.io> on 2015/03/05 22:06:17 UTC
Re: Review Request 31263: Refactored TestAllocator and allocator text
fixture.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31263/
-----------------------------------------------------------
(Updated March 5, 2015, 9:06 p.m.)
Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen.
Changes
-------
Addressed comments.
Bugs: MESOS-2160
https://issues.apache.org/jira/browse/MESOS-2160
Repository: mesos
Description (updated)
-------
TestAllocator stores a pointer to a real allocator and takes over its ownership. MasterAllocatorTest fixture stores the test allocator in turn.
Diffs (updated)
-----
src/tests/containerizer.cpp 26b87ac6b16dfeaf84888e80296ef540697bd775
src/tests/master_allocator_tests.cpp a432d0207e1a92532a495bf9ad2826414ee4f6f0
src/tests/mesos.hpp 45e35204d1aa876fa0c871acf0f21afcd5ababe8
src/tests/resource_offers_tests.cpp 882a9ff4d09aace486182828bf43b643b0d0c519
src/tests/slave_recovery_tests.cpp 53adae0118a26e6d25a9ff20c6374cc8e73275b1
Diff: https://reviews.apache.org/r/31263/diff/
Testing (updated)
-------
make check (Mac OS 10.9.5, Ubuntu 14.04)
Thanks,
Alexander Rukletsov
Re: Review Request 31263: Refactored TestAllocator and allocator text
fixture.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31263/#review76814
-----------------------------------------------------------
Ship it!
Ship It!
- Kapil Arya
On March 13, 2015, 5:49 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31263/
> -----------------------------------------------------------
>
> (Updated March 13, 2015, 5:49 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 a432d0207e1a92532a495bf9ad2826414ee4f6f0
> src/tests/mesos.hpp 45e35204d1aa876fa0c871acf0f21afcd5ababe8
> src/tests/resource_offers_tests.cpp 882a9ff4d09aace486182828bf43b643b0d0c519
> src/tests/slave_recovery_tests.cpp 53adae0118a26e6d25a9ff20c6374cc8e73275b1
>
> Diff: https://reviews.apache.org/r/31263/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.9.5, Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 31263: Refactored TestAllocator and allocator text
fixture.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31263/#review77566
-----------------------------------------------------------
LGTM.
- Michael Park
On March 23, 2015, 2:10 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31263/
> -----------------------------------------------------------
>
> (Updated March 23, 2015, 2:10 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 a432d0207e1a92532a495bf9ad2826414ee4f6f0
> src/tests/mesos.hpp 45e35204d1aa876fa0c871acf0f21afcd5ababe8
> src/tests/resource_offers_tests.cpp 882a9ff4d09aace486182828bf43b643b0d0c519
> src/tests/slave_recovery_tests.cpp 53adae0118a26e6d25a9ff20c6374cc8e73275b1
>
> Diff: https://reviews.apache.org/r/31263/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.9.5, CentOS 7.0)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 31263: Refactored TestAllocator and allocator text
fixture.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31263/#review81040
-----------------------------------------------------------
Ship it!
Ship It!
- Niklas Nielsen
On April 21, 2015, 12:02 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31263/
> -----------------------------------------------------------
>
> (Updated April 21, 2015, 12:02 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 owns a pointer to a real allocator. Each test should instantiate and destroy Allocator instances explicitly to avoid not expected calls.
>
>
> Diffs
> -----
>
> src/tests/containerizer.cpp 26b87ac6b16dfeaf84888e80296ef540697bd775
> src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302
> src/tests/mesos.hpp 7744df55a2a31446327da7bd2b16457e90711d22
> 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
>
>
Re: Review Request 31263: Refactored TestAllocator and allocator text
fixture.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31263/
-----------------------------------------------------------
(Updated April 21, 2015, 7:02 p.m.)
Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen.
Changes
-------
Fixed formatting and a typo.
Bugs: MESOS-2160
https://issues.apache.org/jira/browse/MESOS-2160
Repository: mesos
Description
-------
TestAllocator owns a pointer to a real allocator. Each test should instantiate and destroy Allocator instances explicitly to avoid not expected calls.
Diffs (updated)
-----
src/tests/containerizer.cpp 26b87ac6b16dfeaf84888e80296ef540697bd775
src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302
src/tests/mesos.hpp 7744df55a2a31446327da7bd2b16457e90711d22
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
Re: Review Request 31263: Refactored TestAllocator and allocator text
fixture.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31263/
-----------------------------------------------------------
(Updated April 20, 2015, 1:48 p.m.)
Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen.
Changes
-------
Addressed Vinod's and MPark's comments.
Bugs: MESOS-2160
https://issues.apache.org/jira/browse/MESOS-2160
Repository: mesos
Description (updated)
-------
TestAllocator owns a pointer to a real allocator. Each test should instantiate and destroy Allocator instances explicitly to avoid not expected calls.
Diffs (updated)
-----
src/tests/containerizer.cpp 26b87ac6b16dfeaf84888e80296ef540697bd775
src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302
src/tests/mesos.hpp 7744df55a2a31446327da7bd2b16457e90711d22
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
Re: Review Request 31263: Refactored TestAllocator and allocator text
fixture.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
Rebased.
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 (updated)
-----
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
Re: Review Request 31263: Refactored TestAllocator and allocator text
fixture.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31263/
-----------------------------------------------------------
(Updated April 7, 2015, 12:48 p.m.)
Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen.
Changes
-------
Rebased.
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 (updated)
-----
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
Re: Review Request 31263: Refactored TestAllocator and allocator text
fixture.
Posted by Michael Park <mc...@gmail.com>.
> 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>();
> }
>
> ...
> };
> ```
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
>
>
Re: Review Request 31263: Refactored TestAllocator and allocator text
fixture.
Posted by Michael Park <mc...@gmail.com>.
> 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?
@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
-----------------------------------------------------------
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
>
>
Re: Review Request 31263: Refactored TestAllocator and allocator text
fixture.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> 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
>
>
Re: Review Request 31263: Refactored TestAllocator and allocator text
fixture.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> 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
>
>
Re: Review Request 31263: Refactored TestAllocator and allocator text
fixture.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31263/#review78710
-----------------------------------------------------------
src/tests/master_allocator_tests.cpp
<https://reviews.apache.org/r/31263/#comment127666>
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;
};
```
- Vinod Kone
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
>
>
Re: Review Request 31263: Refactored TestAllocator and allocator text
fixture.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
Rebased.
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 (updated)
-----
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
Re: Review Request 31263: Refactored TestAllocator and allocator text
fixture.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31263/
-----------------------------------------------------------
(Updated March 27, 2015, 4:27 p.m.)
Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen.
Changes
-------
Rebased.
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 (updated)
-----
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
Re: Review Request 31263: Refactored TestAllocator and allocator text
fixture.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31263/
-----------------------------------------------------------
(Updated March 23, 2015, 2:10 p.m.)
Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen.
Changes
-------
Introduced paranthesis for consistency.
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 (updated)
-----
src/tests/containerizer.cpp 26b87ac6b16dfeaf84888e80296ef540697bd775
src/tests/master_allocator_tests.cpp a432d0207e1a92532a495bf9ad2826414ee4f6f0
src/tests/mesos.hpp 45e35204d1aa876fa0c871acf0f21afcd5ababe8
src/tests/resource_offers_tests.cpp 882a9ff4d09aace486182828bf43b643b0d0c519
src/tests/slave_recovery_tests.cpp 53adae0118a26e6d25a9ff20c6374cc8e73275b1
Diff: https://reviews.apache.org/r/31263/diff/
Testing (updated)
-------
make check (Mac OS 10.9.5, CentOS 7.0)
Thanks,
Alexander Rukletsov
Re: Review Request 31263: Refactored TestAllocator and allocator text
fixture.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31263/
-----------------------------------------------------------
(Updated March 13, 2015, 9:49 p.m.)
Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen.
Changes
-------
Addressed Niklas' comments.
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 (updated)
-----
src/tests/containerizer.cpp 26b87ac6b16dfeaf84888e80296ef540697bd775
src/tests/master_allocator_tests.cpp a432d0207e1a92532a495bf9ad2826414ee4f6f0
src/tests/mesos.hpp 45e35204d1aa876fa0c871acf0f21afcd5ababe8
src/tests/resource_offers_tests.cpp 882a9ff4d09aace486182828bf43b643b0d0c519
src/tests/slave_recovery_tests.cpp 53adae0118a26e6d25a9ff20c6374cc8e73275b1
Diff: https://reviews.apache.org/r/31263/diff/
Testing
-------
make check (Mac OS 10.9.5, Ubuntu 14.04)
Thanks,
Alexander Rukletsov
Re: Review Request 31263: Refactored TestAllocator and allocator text
fixture.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On March 10, 2015, 9:10 p.m., Niklas Nielsen wrote:
> > src/tests/mesos.hpp, line 770
> > <https://reviews.apache.org/r/31263/diff/3/?file=886222#file886222line770>
> >
> > Do you want to have:
> >
> > CHECK_NOTNULL(allocator->real)->initialize(arg0, arg1, arg2);
> >
> > ?
We do not CHECK_NOTNULL(allocator) either. I suggest we either do both, or I can introduce a `.real()` getter that does the check and returns an `Allocator&`.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31263/#review75970
-----------------------------------------------------------
On March 5, 2015, 9:11 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31263/
> -----------------------------------------------------------
>
> (Updated March 5, 2015, 9:11 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 a432d0207e1a92532a495bf9ad2826414ee4f6f0
> src/tests/mesos.hpp 45e35204d1aa876fa0c871acf0f21afcd5ababe8
> src/tests/resource_offers_tests.cpp 882a9ff4d09aace486182828bf43b643b0d0c519
> src/tests/slave_recovery_tests.cpp 53adae0118a26e6d25a9ff20c6374cc8e73275b1
>
> Diff: https://reviews.apache.org/r/31263/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.9.5, Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 31263: Refactored TestAllocator and allocator text
fixture.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On March 10, 2015, 9:10 p.m., Niklas Nielsen wrote:
> > src/tests/mesos.hpp, line 1021
> > <https://reviews.apache.org/r/31263/diff/3/?file=886222#file886222line1021>
> >
> > Can you reintroduce this destructor?
It is redundant, since we have a d-tor in `mesos::master::allocator::Allocator`, but I can bring it back for consistency.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31263/#review75970
-----------------------------------------------------------
On March 5, 2015, 9:11 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31263/
> -----------------------------------------------------------
>
> (Updated March 5, 2015, 9:11 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 a432d0207e1a92532a495bf9ad2826414ee4f6f0
> src/tests/mesos.hpp 45e35204d1aa876fa0c871acf0f21afcd5ababe8
> src/tests/resource_offers_tests.cpp 882a9ff4d09aace486182828bf43b643b0d0c519
> src/tests/slave_recovery_tests.cpp 53adae0118a26e6d25a9ff20c6374cc8e73275b1
>
> Diff: https://reviews.apache.org/r/31263/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.9.5, Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 31263: Refactored TestAllocator and allocator text
fixture.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31263/#review75970
-----------------------------------------------------------
src/tests/master_allocator_tests.cpp
<https://reviews.apache.org/r/31263/#comment123322>
Can you add a comment about why you don't return a pointer here?
src/tests/master_allocator_tests.cpp
<https://reviews.apache.org/r/31263/#comment123323>
s/!//
src/tests/master_allocator_tests.cpp
<https://reviews.apache.org/r/31263/#comment123326>
We need a default value for testAllocator
src/tests/mesos.hpp
<https://reviews.apache.org/r/31263/#comment123328>
Do you want to have:
CHECK_NOTNULL(allocator->real)->initialize(arg0, arg1, arg2);
?
src/tests/mesos.hpp
<https://reviews.apache.org/r/31263/#comment123332>
Let's see if we can merge the two constructors
src/tests/mesos.hpp
<https://reviews.apache.org/r/31263/#comment123336>
With one constructor, we could get rid of this helper.
src/tests/mesos.hpp
<https://reviews.apache.org/r/31263/#comment123334>
Can you reintroduce this destructor?
- Niklas Nielsen
On March 5, 2015, 1:11 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31263/
> -----------------------------------------------------------
>
> (Updated March 5, 2015, 1:11 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 a432d0207e1a92532a495bf9ad2826414ee4f6f0
> src/tests/mesos.hpp 45e35204d1aa876fa0c871acf0f21afcd5ababe8
> src/tests/resource_offers_tests.cpp 882a9ff4d09aace486182828bf43b643b0d0c519
> src/tests/slave_recovery_tests.cpp 53adae0118a26e6d25a9ff20c6374cc8e73275b1
>
> Diff: https://reviews.apache.org/r/31263/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.9.5, Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 31263: Refactored TestAllocator and allocator text
fixture.
Posted by Joerg Schad <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31263/#review75403
-----------------------------------------------------------
Ship it!
Ship It!
- Joerg Schad
On March 5, 2015, 9:11 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31263/
> -----------------------------------------------------------
>
> (Updated March 5, 2015, 9:11 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 a432d0207e1a92532a495bf9ad2826414ee4f6f0
> src/tests/mesos.hpp 45e35204d1aa876fa0c871acf0f21afcd5ababe8
> src/tests/resource_offers_tests.cpp 882a9ff4d09aace486182828bf43b643b0d0c519
> src/tests/slave_recovery_tests.cpp 53adae0118a26e6d25a9ff20c6374cc8e73275b1
>
> Diff: https://reviews.apache.org/r/31263/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.9.5, Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 31263: Refactored TestAllocator and allocator text
fixture.
Posted by Alexander Rukletsov <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31263/
-----------------------------------------------------------
(Updated March 5, 2015, 9:11 p.m.)
Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen.
Changes
-------
Fixed style issue.
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 (updated)
-----
src/tests/containerizer.cpp 26b87ac6b16dfeaf84888e80296ef540697bd775
src/tests/master_allocator_tests.cpp a432d0207e1a92532a495bf9ad2826414ee4f6f0
src/tests/mesos.hpp 45e35204d1aa876fa0c871acf0f21afcd5ababe8
src/tests/resource_offers_tests.cpp 882a9ff4d09aace486182828bf43b643b0d0c519
src/tests/slave_recovery_tests.cpp 53adae0118a26e6d25a9ff20c6374cc8e73275b1
Diff: https://reviews.apache.org/r/31263/diff/
Testing
-------
make check (Mac OS 10.9.5, Ubuntu 14.04)
Thanks,
Alexander Rukletsov