You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Benjamin Hindman <be...@berkeley.edu> on 2012/07/02 06:54:36 UTC
Re: Review Request: Allocator tests
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5532/#review8779
-----------------------------------------------------------
This is cleaning up really nicely!
src/tests/allocator_tests.cpp
<https://reviews.apache.org/r/5532/#comment18570>
Is this Return() necessary?
src/tests/allocator_tests.cpp
<https://reviews.apache.org/r/5532/#comment18571>
Is this Return() necessary?
src/tests/allocator_tests.cpp
<https://reviews.apache.org/r/5532/#comment18564>
Hmm, I didn't fully get this comment. I think you want to say that the first time you _don't_ invoke resourcesRecovered, but you _wait_ until frameworkRemoved is called to test that doing the dispatch after frameworkRemoved is okay (more or less parroting what the comment above the test says).
src/tests/allocator_tests.cpp
<https://reviews.apache.org/r/5532/#comment18566>
Again, remind the reader of the test that we explicitly do this dispatch on behalf of the master to make sure that the allocator can handle getting resources recovered after the framework has been removed.
src/tests/allocator_tests.cpp
<https://reviews.apache.org/r/5532/#comment18565>
Wrap this line please.
src/tests/allocator_tests.cpp
<https://reviews.apache.org/r/5532/#comment18568>
'{' on newline.
src/tests/allocator_tests.cpp
<https://reviews.apache.org/r/5532/#comment18577>
So, I think the right thing to do here is make this protected not public. In addition, I think you want to add a static void SetUpTestCase() here that just calls BaseZooKeeperTest::SetUpTestCase(), and make BaseZooKeeperTest::SetUpTestCase be protected again.
src/tests/allocator_tests.cpp
<https://reviews.apache.org/r/5532/#comment18567>
'{' on new line.
src/tests/allocator_tests.cpp
<https://reviews.apache.org/r/5532/#comment18569>
Please use braces.
src/tests/allocator_tests.cpp
<https://reviews.apache.org/r/5532/#comment18572>
Please use braces.
src/tests/allocator_tests.cpp
<https://reviews.apache.org/r/5532/#comment18573>
Is .Times(2) necessary if you have two WillOnce?
src/tests/allocator_tests.cpp
<https://reviews.apache.org/r/5532/#comment18574>
Sometimes you do .WillRepeatedly(Return()) and sometimes you do .Times(AnyNumber()). Are there semantic differences that keep you from being consistent and just picking one?
src/tests/allocator_tests.cpp
<https://reviews.apache.org/r/5532/#comment18575>
Braces please. But instead of this if check (and the ones above), please turn these all into CHECK(!someTry.isError()) << "error message" << someTry.error();
src/tests/utils.hpp
<https://reviews.apache.org/r/5532/#comment18578>
I love this factored out and useable by others, but I don't love implicitly passing arguments through environment variables. How hard would it be to explicitly pass the arguments to this function? As in, make the signature something like:
launchTask(SchedulerDriver* driver, const std::vector<Offer>& offers, double cpus, double mem)
And maybe even take the total number of tasks to launch?
src/tests/utils.hpp
<https://reviews.apache.org/r/5532/#comment18576>
driver->declineOffer(offer.id());
- Benjamin Hindman
On June 28, 2012, 10:19 p.m., Thomas Marshall wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5532/
> -----------------------------------------------------------
>
> (Updated June 28, 2012, 10:19 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> Added tests for allocators.
>
> This patch depends on two other currently pending review requests:
> https://reviews.apache.org/r/5448/
> https://reviews.apache.org/r/5451/
>
>
> This addresses bug MESOS-224.
> https://issues.apache.org/jira/browse/MESOS-224
>
>
> Diffs
> -----
>
> src/Makefile.am 8760f59
> src/common/type_utils.hpp acf3e65
> src/tests/allocator_tests.cpp PRE-CREATION
> src/tests/base_zookeeper_test.hpp 4613376
> src/tests/resource_offers_tests.cpp c1f1760
> src/tests/utils.hpp e81ec82
> src/tests/utils.cpp e7cda40
>
> Diff: https://reviews.apache.org/r/5532/diff/
>
>
> Testing
> -------
>
> make check on Lion
>
>
> Thanks,
>
> Thomas Marshall
>
>
Re: Review Request: Allocator tests
Posted by Thomas Marshall <tw...@gmail.com>.
> On July 2, 2012, 4:54 a.m., Benjamin Hindman wrote:
> > src/tests/allocator_tests.cpp, line 685
> > <https://reviews.apache.org/r/5532/diff/4/?file=117150#file117150line685>
> >
> > Sometimes you do .WillRepeatedly(Return()) and sometimes you do .Times(AnyNumber()). Are there semantic differences that keep you from being consistent and just picking one?
The difference is that WillRepeatedly(Return()) won't do the default whereas Times(AnyNumber()) will. I'll change all of the Times(AnyNumber()) to WillRepeatedly(DoDefault()) to make this clearer.
> On July 2, 2012, 4:54 a.m., Benjamin Hindman wrote:
> > src/tests/allocator_tests.cpp, line 608
> > <https://reviews.apache.org/r/5532/diff/4/?file=117150#file117150line608>
> >
> > So, I think the right thing to do here is make this protected not public. In addition, I think you want to add a static void SetUpTestCase() here that just calls BaseZooKeeperTest::SetUpTestCase(), and make BaseZooKeeperTest::SetUpTestCase be protected again.
Apparently SetUpTestCase must be public for TYPED_TESTs (like not putting DoDefaults in DoAlls, it's for "technical reasons"), but at the very least I can make BaseZooKeeperTest::SetUpTestCase protected by declaring a public MasterFailoverAllocatorTest::SetUpTestCase which calls it.
> On July 2, 2012, 4:54 a.m., Benjamin Hindman wrote:
> > src/tests/allocator_tests.cpp, line 393
> > <https://reviews.apache.org/r/5532/diff/4/?file=117150#file117150line393>
> >
> > Is this Return() necessary?
As far as I can tell, the compiler doesn't recognize DoAll unless it contains at least one predefined gmock action, as opposed to custom actions we defined. I can't find any documentation on this, but I've asked the Google Mock Group about it, so I'll leave it in for now and fix it later if they get back to me with a solution.
- Thomas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5532/#review8779
-----------------------------------------------------------
On June 28, 2012, 10:19 p.m., Thomas Marshall wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5532/
> -----------------------------------------------------------
>
> (Updated June 28, 2012, 10:19 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> Added tests for allocators.
>
> This patch depends on two other currently pending review requests:
> https://reviews.apache.org/r/5448/
> https://reviews.apache.org/r/5451/
>
>
> This addresses bug MESOS-224.
> https://issues.apache.org/jira/browse/MESOS-224
>
>
> Diffs
> -----
>
> src/Makefile.am 8760f59
> src/common/type_utils.hpp acf3e65
> src/tests/allocator_tests.cpp PRE-CREATION
> src/tests/base_zookeeper_test.hpp 4613376
> src/tests/resource_offers_tests.cpp c1f1760
> src/tests/utils.hpp e81ec82
> src/tests/utils.cpp e7cda40
>
> Diff: https://reviews.apache.org/r/5532/diff/
>
>
> Testing
> -------
>
> make check on Lion
>
>
> Thanks,
>
> Thomas Marshall
>
>