You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Thomas Marshall <tw...@gmail.com> on 2013/04/16 20:05:11 UTC
Review Request: Update allocator tests to use the cluster abstraction
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10552/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
Description
-------
Update allocator tests to use cluster abstraction. Also includes fixes for a few bugs that were uncovered:
- We now create separate isolators and executors for each slave in tests with multiple slaves.
- Tests that launched a task and then immediately launched another framework without waiting for the task were seg faulting occasionally on Ubuntu when TestingIsolator::launchExecutor was modifying the environment while MesosSchedulerDriver::MesosSchedulerDriver was trying to read the environment. We now wait for the task to finish launching before starting the second framework in these tests.
- Some tests have been significantly sped up by setting the allocation_interval flag for the master from the default 1s to 50ms.
- The MockAllocator was reintroduced into the DRFAllocatorTest because it turns out that its necessary to ensure that the frameworks and slaves are added to the allocator in the correct order.
- The AllocatorZookeeperTests were occasionally failing on waiting for the first allocation because it took longer than the 2s that AWAIT_UNTIL waits. I added another AWAIT_UNTIL on a slightly earlier event to break up this wait and ensure its always under 2s.
I also added another Cluster::Masters::start that takes both an AllocatorProcess and master::Flags.
Diffs
-----
src/tests/allocator_tests.cpp 85a4981
src/tests/allocator_zookeeper_tests.cpp 82a498e
src/tests/cluster.hpp 28fc269
Diff: https://reviews.apache.org/r/10552/diff/
Testing
-------
bin/mesos-tests.sh --gtest_break_on_failure --gtest_repeat=3000 --gtest_filter=*Allocator*
Thanks,
Thomas Marshall
Re: Review Request: Update allocator tests to use the cluster abstraction
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10552/#review19288
-----------------------------------------------------------
Ship it!
In the future, it would be great if reviews like these could be split into multiple bug fixes. From your description, sounded like it wouldn't have been that hard in this case? Anyway, just wanted to note that.
FWIW, we have a script called "support/post-reviews.py" that makes it easy to send multiple reviews from a branch (one for each commit).
src/tests/allocator_tests.cpp
<https://reviews.apache.org/r/10552/#comment39954>
thank you
- Vinod Kone
On April 16, 2013, 10:16 p.m., Thomas Marshall wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10552/
> -----------------------------------------------------------
>
> (Updated April 16, 2013, 10:16 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> Update allocator tests to use cluster abstraction. Also includes fixes for a few bugs that were uncovered:
>
> - We now create separate isolators and executors for each slave in tests with multiple slaves.
> - Tests that launched a task and then immediately launched another framework without waiting for the task were seg faulting occasionally on Ubuntu when TestingIsolator::launchExecutor was modifying the environment while MesosSchedulerDriver::MesosSchedulerDriver was trying to read the environment. We now wait for the task to finish launching before starting the second framework in these tests.
> - Some tests have been significantly sped up by setting the allocation_interval flag for the master from the default 1s to 50ms.
> - The MockAllocator was reintroduced into the DRFAllocatorTest because it turns out that its necessary to ensure that the frameworks and slaves are added to the allocator in the correct order.
> - The AllocatorZookeeperTests were occasionally failing on waiting for the first allocation because it took longer than the 2s that AWAIT_UNTIL waits. I added another AWAIT_UNTIL on a slightly earlier event to break up this wait and ensure its always under 2s.
>
> I also added another Cluster::Masters::start that takes both an AllocatorProcess and master::Flags.
>
>
> Diffs
> -----
>
> src/tests/allocator_tests.cpp 85a4981
> src/tests/allocator_zookeeper_tests.cpp 82a498e
> src/tests/cluster.hpp 28fc269
>
> Diff: https://reviews.apache.org/r/10552/diff/
>
>
> Testing
> -------
>
> bin/mesos-tests.sh --gtest_break_on_failure --gtest_repeat=3000 --gtest_filter=*Allocator*
>
>
> Thanks,
>
> Thomas Marshall
>
>
Re: Review Request: Update allocator tests to use the cluster abstraction
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10552/#review19378
-----------------------------------------------------------
committed to trunk. please mark as submitted.
- Vinod Kone
On April 16, 2013, 10:16 p.m., Thomas Marshall wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10552/
> -----------------------------------------------------------
>
> (Updated April 16, 2013, 10:16 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> Update allocator tests to use cluster abstraction. Also includes fixes for a few bugs that were uncovered:
>
> - We now create separate isolators and executors for each slave in tests with multiple slaves.
> - Tests that launched a task and then immediately launched another framework without waiting for the task were seg faulting occasionally on Ubuntu when TestingIsolator::launchExecutor was modifying the environment while MesosSchedulerDriver::MesosSchedulerDriver was trying to read the environment. We now wait for the task to finish launching before starting the second framework in these tests.
> - Some tests have been significantly sped up by setting the allocation_interval flag for the master from the default 1s to 50ms.
> - The MockAllocator was reintroduced into the DRFAllocatorTest because it turns out that its necessary to ensure that the frameworks and slaves are added to the allocator in the correct order.
> - The AllocatorZookeeperTests were occasionally failing on waiting for the first allocation because it took longer than the 2s that AWAIT_UNTIL waits. I added another AWAIT_UNTIL on a slightly earlier event to break up this wait and ensure its always under 2s.
>
> I also added another Cluster::Masters::start that takes both an AllocatorProcess and master::Flags.
>
>
> Diffs
> -----
>
> src/tests/allocator_tests.cpp 85a4981
> src/tests/allocator_zookeeper_tests.cpp 82a498e
> src/tests/cluster.hpp 28fc269
>
> Diff: https://reviews.apache.org/r/10552/diff/
>
>
> Testing
> -------
>
> bin/mesos-tests.sh --gtest_break_on_failure --gtest_repeat=3000 --gtest_filter=*Allocator*
>
>
> Thanks,
>
> Thomas Marshall
>
>
Re: Review Request: Update allocator tests to use the cluster abstraction
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10552/#review19377
-----------------------------------------------------------
committed to trunk. please mark as submitted.
- Vinod Kone
On April 16, 2013, 10:16 p.m., Thomas Marshall wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10552/
> -----------------------------------------------------------
>
> (Updated April 16, 2013, 10:16 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> Update allocator tests to use cluster abstraction. Also includes fixes for a few bugs that were uncovered:
>
> - We now create separate isolators and executors for each slave in tests with multiple slaves.
> - Tests that launched a task and then immediately launched another framework without waiting for the task were seg faulting occasionally on Ubuntu when TestingIsolator::launchExecutor was modifying the environment while MesosSchedulerDriver::MesosSchedulerDriver was trying to read the environment. We now wait for the task to finish launching before starting the second framework in these tests.
> - Some tests have been significantly sped up by setting the allocation_interval flag for the master from the default 1s to 50ms.
> - The MockAllocator was reintroduced into the DRFAllocatorTest because it turns out that its necessary to ensure that the frameworks and slaves are added to the allocator in the correct order.
> - The AllocatorZookeeperTests were occasionally failing on waiting for the first allocation because it took longer than the 2s that AWAIT_UNTIL waits. I added another AWAIT_UNTIL on a slightly earlier event to break up this wait and ensure its always under 2s.
>
> I also added another Cluster::Masters::start that takes both an AllocatorProcess and master::Flags.
>
>
> Diffs
> -----
>
> src/tests/allocator_tests.cpp 85a4981
> src/tests/allocator_zookeeper_tests.cpp 82a498e
> src/tests/cluster.hpp 28fc269
>
> Diff: https://reviews.apache.org/r/10552/diff/
>
>
> Testing
> -------
>
> bin/mesos-tests.sh --gtest_break_on_failure --gtest_repeat=3000 --gtest_filter=*Allocator*
>
>
> Thanks,
>
> Thomas Marshall
>
>
Re: Review Request: Update allocator tests to use the cluster abstraction
Posted by Thomas Marshall <tw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10552/
-----------------------------------------------------------
(Updated April 16, 2013, 10:16 p.m.)
Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
Changes
-------
Ben's review.
Description
-------
Update allocator tests to use cluster abstraction. Also includes fixes for a few bugs that were uncovered:
- We now create separate isolators and executors for each slave in tests with multiple slaves.
- Tests that launched a task and then immediately launched another framework without waiting for the task were seg faulting occasionally on Ubuntu when TestingIsolator::launchExecutor was modifying the environment while MesosSchedulerDriver::MesosSchedulerDriver was trying to read the environment. We now wait for the task to finish launching before starting the second framework in these tests.
- Some tests have been significantly sped up by setting the allocation_interval flag for the master from the default 1s to 50ms.
- The MockAllocator was reintroduced into the DRFAllocatorTest because it turns out that its necessary to ensure that the frameworks and slaves are added to the allocator in the correct order.
- The AllocatorZookeeperTests were occasionally failing on waiting for the first allocation because it took longer than the 2s that AWAIT_UNTIL waits. I added another AWAIT_UNTIL on a slightly earlier event to break up this wait and ensure its always under 2s.
I also added another Cluster::Masters::start that takes both an AllocatorProcess and master::Flags.
Diffs (updated)
-----
src/tests/allocator_tests.cpp 85a4981
src/tests/allocator_zookeeper_tests.cpp 82a498e
src/tests/cluster.hpp 28fc269
Diff: https://reviews.apache.org/r/10552/diff/
Testing
-------
bin/mesos-tests.sh --gtest_break_on_failure --gtest_repeat=3000 --gtest_filter=*Allocator*
Thanks,
Thomas Marshall
Re: Review Request: Update allocator tests to use the cluster abstraction
Posted by Thomas Marshall <tw...@gmail.com>.
> On April 16, 2013, 9:36 p.m., Vinod Kone wrote:
> > src/tests/allocator_tests.cpp, lines 454-459
> > <https://reviews.apache.org/r/10552/diff/1/?file=281596#file281596line454>
> >
> > Any specific reason you followed this pattern here and everywhere else?
> >
> > Would it be sufficient to just EXPECT_CALL(slaveRemoved).Atmost(1)
> > and
> > clusters.shutdown()
> >
> > just curious?
> >
Since these tests are for the allocator, it seemed nicest to have the expectations on the allocator be as specific as possible. You're correct, though, that AtMost(1) will work, and since it makes the tests slightly cleaner and faster I'll change all of them except in the simplest test, MockAllocator, just to keep a test that ensures that the master really does send the slaveRemoved call.
- Thomas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10552/#review19273
-----------------------------------------------------------
On April 16, 2013, 6:05 p.m., Thomas Marshall wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10552/
> -----------------------------------------------------------
>
> (Updated April 16, 2013, 6:05 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> Update allocator tests to use cluster abstraction. Also includes fixes for a few bugs that were uncovered:
>
> - We now create separate isolators and executors for each slave in tests with multiple slaves.
> - Tests that launched a task and then immediately launched another framework without waiting for the task were seg faulting occasionally on Ubuntu when TestingIsolator::launchExecutor was modifying the environment while MesosSchedulerDriver::MesosSchedulerDriver was trying to read the environment. We now wait for the task to finish launching before starting the second framework in these tests.
> - Some tests have been significantly sped up by setting the allocation_interval flag for the master from the default 1s to 50ms.
> - The MockAllocator was reintroduced into the DRFAllocatorTest because it turns out that its necessary to ensure that the frameworks and slaves are added to the allocator in the correct order.
> - The AllocatorZookeeperTests were occasionally failing on waiting for the first allocation because it took longer than the 2s that AWAIT_UNTIL waits. I added another AWAIT_UNTIL on a slightly earlier event to break up this wait and ensure its always under 2s.
>
> I also added another Cluster::Masters::start that takes both an AllocatorProcess and master::Flags.
>
>
> Diffs
> -----
>
> src/tests/allocator_tests.cpp 85a4981
> src/tests/allocator_zookeeper_tests.cpp 82a498e
> src/tests/cluster.hpp 28fc269
>
> Diff: https://reviews.apache.org/r/10552/diff/
>
>
> Testing
> -------
>
> bin/mesos-tests.sh --gtest_break_on_failure --gtest_repeat=3000 --gtest_filter=*Allocator*
>
>
> Thanks,
>
> Thomas Marshall
>
>
Re: Review Request: Update allocator tests to use the cluster abstraction
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10552/#review19273
-----------------------------------------------------------
src/tests/allocator_tests.cpp
<https://reviews.apache.org/r/10552/#comment39914>
This test is very complex. Would you expand on this comment so we can understand whats happening?
src/tests/allocator_tests.cpp
<https://reviews.apache.org/r/10552/#comment39915>
Any specific reason you followed this pattern here and everywhere else?
Would it be sufficient to just EXPECT_CALL(slaveRemoved).Atmost(1)
and
clusters.shutdown()
just curious?
src/tests/allocator_tests.cpp
<https://reviews.apache.org/r/10552/#comment39917>
Can you replace all AWAIT_UNTIL's with AWAIT_READY's?
The latter checks that the future is Ready().
- Vinod Kone
On April 16, 2013, 6:05 p.m., Thomas Marshall wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10552/
> -----------------------------------------------------------
>
> (Updated April 16, 2013, 6:05 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> Update allocator tests to use cluster abstraction. Also includes fixes for a few bugs that were uncovered:
>
> - We now create separate isolators and executors for each slave in tests with multiple slaves.
> - Tests that launched a task and then immediately launched another framework without waiting for the task were seg faulting occasionally on Ubuntu when TestingIsolator::launchExecutor was modifying the environment while MesosSchedulerDriver::MesosSchedulerDriver was trying to read the environment. We now wait for the task to finish launching before starting the second framework in these tests.
> - Some tests have been significantly sped up by setting the allocation_interval flag for the master from the default 1s to 50ms.
> - The MockAllocator was reintroduced into the DRFAllocatorTest because it turns out that its necessary to ensure that the frameworks and slaves are added to the allocator in the correct order.
> - The AllocatorZookeeperTests were occasionally failing on waiting for the first allocation because it took longer than the 2s that AWAIT_UNTIL waits. I added another AWAIT_UNTIL on a slightly earlier event to break up this wait and ensure its always under 2s.
>
> I also added another Cluster::Masters::start that takes both an AllocatorProcess and master::Flags.
>
>
> Diffs
> -----
>
> src/tests/allocator_tests.cpp 85a4981
> src/tests/allocator_zookeeper_tests.cpp 82a498e
> src/tests/cluster.hpp 28fc269
>
> Diff: https://reviews.apache.org/r/10552/diff/
>
>
> Testing
> -------
>
> bin/mesos-tests.sh --gtest_break_on_failure --gtest_repeat=3000 --gtest_filter=*Allocator*
>
>
> Thanks,
>
> Thomas Marshall
>
>