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 2013/05/21 00:06:59 UTC
Review Request: Refactored MesosTest/MesosClusterTest into a generic fixture
for launching in-memory Mesos clusters and updated all tests appropriately.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11273/
-----------------------------------------------------------
Review request for mesos and Vinod Kone.
Description
-------
This looks bigger than it actually is. ;)
Diffs
-----
src/Makefile.am 343a7c5288e83d20dc6dd4db5353499199b90cb1
src/master/hierarchical_allocator_process.hpp 2422fbdc49fd575b615e73891e3cda6675358d2d
src/tests/allocator_tests.cpp 540c05afa54caafea7c0b49773cf2613c87d1ba5
src/tests/allocator_zookeeper_tests.cpp 2c7deb1ef8d4349964ffa0bdd7b97ae616be1c43
src/tests/cgroups_tests.cpp 3734315360fc1fb33a487696123a685aa18c9620
src/tests/cluster.hpp 682b7d6f06e798b0ae7b676b1cdc9df14dd6f414
src/tests/configurator_tests.cpp e8ba9362a462fda6774db65bbd633b6b1da36c8c
src/tests/exception_tests.cpp f405a61a7feb73e96b347d6c8e6fd316bdf6a1aa
src/tests/fault_tolerance_tests.cpp 68cd5fcff266d102d8edf265776517aa2eef6061
src/tests/files_tests.cpp 5679ecd3a0e6c8cf7db0e8e97c1c2d2d570caf84
src/tests/flags_tests.cpp e07dbcc25552351f64d644130ca09ed9b15a988b
src/tests/gc_tests.cpp 97d2685e0f89e059ad424ecc5887eb18acc9612c
src/tests/group_tests.cpp PRE-CREATION
src/tests/isolator.hpp 17dc7b30415be1db984ca6290213840770f465a5
src/tests/isolator_tests.cpp 435c780f05b5bc78d1dc4e9cbcbac74971607dc9
src/tests/log_tests.cpp 57fb9db5ef79f5e285eb2624bdfe25375fb8598e
src/tests/logging_tests.cpp 57eae79724c9952c412ff3c23d36fe9d54356b5b
src/tests/main.cpp e32ec0cac7b7ab9b31f4e419d0c1426b2c8aeb65
src/tests/master_detector_tests.cpp 521f0279b165f5d3639788ff6a00b772e6f71b7a
src/tests/master_tests.cpp 1b062d9ce96ae223389dea5f030f51640511ad52
src/tests/mesos.hpp PRE-CREATION
src/tests/mesos.cpp PRE-CREATION
src/tests/monitor_tests.cpp 9973f98c35415cb72e8f5124bf4d7ff402490a39
src/tests/paths_tests.cpp 84ad9b385cccecdd3ba61366e67c493926588e23
src/tests/protobuf_io_tests.cpp b6478b3140aee69741a3e62485260f3661835c41
src/tests/reaper_tests.cpp 0809c1ff17eb949beb1bdd922fdced022aa202f3
src/tests/resource_offers_tests.cpp 5e53a89bcf99dedd4535545dc44a1a6229a09b0f
src/tests/script.cpp d7f103eb08335638bca57ccab77646534342b058
src/tests/slave_recovery_tests.cpp d963ce1251ca28a832ca5d3e00c40aa7911ccb03
src/tests/state_tests.cpp eb124fdc10491f085007b5a7e9fd47e006f1523f
src/tests/status_update_manager_tests.cpp e375898152de357b1b50b7c5185b825e51bfaa3e
src/tests/utils.hpp ca3ecd7f0cab283327bf83e57d4b405b4ada9c74
src/tests/utils.cpp 9d1d5ad1e192b0f44a7a7173839b292f6bedad15
src/tests/zookeeper.hpp PRE-CREATION
src/tests/zookeeper.cpp PRE-CREATION
src/tests/zookeeper_tests.cpp e9180bfd35ee4477849d3a776c29eff5206d66e9
src/zookeeper/authentication.hpp fb0e3ca47f0a0327d6358b9e175e1a0452110bf7
src/zookeeper/url.hpp b297a16e0da54892bf7dd79bbe077f70e1dc9c06
Diff: https://reviews.apache.org/r/11273/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request: Refactored MesosTest/MesosClusterTest into a generic
fixture for launching in-memory Mesos clusters and updated all tests
appropriately.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On May 24, 2013, 1:54 a.m., Vinod Kone wrote:
> > src/master/hierarchical_allocator_process.hpp, line 36
> > <https://reviews.apache.org/r/11273/diff/1/?file=294304#file294304line36>
> >
> > hmm..why the revert?
Not sure what you mean by "revert", but I did move this up to the other includes.
> On May 24, 2013, 1:54 a.m., Vinod Kone wrote:
> > src/tests/cgroups_tests.cpp, line 50
> > <https://reviews.apache.org/r/11273/diff/1/?file=294307#file294307line50>
> >
> > s/HIERARCHY|ROOT/{HIERARCHY|ROOT}/
I like it. Although, if we want to be regular expression pedantic should we use () instead of {}?
> On May 24, 2013, 1:54 a.m., Vinod Kone wrote:
> > src/tests/mesos.hpp, lines 117-118
> > <https://reviews.apache.org/r/11273/diff/1/?file=294323#file294323line117>
> >
> > Can you pull this up to match google test's style?
> >
> > https://code.google.com/p/googletest/source/browse/trunk/samples/sample3_unittest.cc
If by "style" you mean put those functions before the others, sure. Is that what you meant?
> On May 24, 2013, 1:54 a.m., Vinod Kone wrote:
> > src/tests/mesos.hpp, line 122
> > <https://reviews.apache.org/r/11273/diff/1/?file=294323#file294323line122>
> >
> > Can you add a comment here?
Done.
> On May 24, 2013, 1:54 a.m., Vinod Kone wrote:
> > src/tests/mesos.cpp, line 164
> > <https://reviews.apache.org/r/11273/diff/1/?file=294324#file294324line164>
> >
> > yup. that would be swell.
This is a bit more involved because it requires adding state that knows whether or not the cluster has been shut down, so I'll keep it a TODO for now.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11273/#review20973
-----------------------------------------------------------
On May 20, 2013, 10:06 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11273/
> -----------------------------------------------------------
>
> (Updated May 20, 2013, 10:06 p.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Description
> -------
>
> This looks bigger than it actually is. ;)
>
>
> Diffs
> -----
>
> src/Makefile.am 343a7c5288e83d20dc6dd4db5353499199b90cb1
> src/master/hierarchical_allocator_process.hpp 2422fbdc49fd575b615e73891e3cda6675358d2d
> src/tests/allocator_tests.cpp 540c05afa54caafea7c0b49773cf2613c87d1ba5
> src/tests/allocator_zookeeper_tests.cpp 2c7deb1ef8d4349964ffa0bdd7b97ae616be1c43
> src/tests/cgroups_tests.cpp 3734315360fc1fb33a487696123a685aa18c9620
> src/tests/cluster.hpp 682b7d6f06e798b0ae7b676b1cdc9df14dd6f414
> src/tests/configurator_tests.cpp e8ba9362a462fda6774db65bbd633b6b1da36c8c
> src/tests/exception_tests.cpp f405a61a7feb73e96b347d6c8e6fd316bdf6a1aa
> src/tests/fault_tolerance_tests.cpp 68cd5fcff266d102d8edf265776517aa2eef6061
> src/tests/files_tests.cpp 5679ecd3a0e6c8cf7db0e8e97c1c2d2d570caf84
> src/tests/flags_tests.cpp e07dbcc25552351f64d644130ca09ed9b15a988b
> src/tests/gc_tests.cpp 97d2685e0f89e059ad424ecc5887eb18acc9612c
> src/tests/group_tests.cpp PRE-CREATION
> src/tests/isolator.hpp 17dc7b30415be1db984ca6290213840770f465a5
> src/tests/isolator_tests.cpp 435c780f05b5bc78d1dc4e9cbcbac74971607dc9
> src/tests/log_tests.cpp 57fb9db5ef79f5e285eb2624bdfe25375fb8598e
> src/tests/logging_tests.cpp 57eae79724c9952c412ff3c23d36fe9d54356b5b
> src/tests/main.cpp e32ec0cac7b7ab9b31f4e419d0c1426b2c8aeb65
> src/tests/master_detector_tests.cpp 521f0279b165f5d3639788ff6a00b772e6f71b7a
> src/tests/master_tests.cpp 1b062d9ce96ae223389dea5f030f51640511ad52
> src/tests/mesos.hpp PRE-CREATION
> src/tests/mesos.cpp PRE-CREATION
> src/tests/monitor_tests.cpp 9973f98c35415cb72e8f5124bf4d7ff402490a39
> src/tests/paths_tests.cpp 84ad9b385cccecdd3ba61366e67c493926588e23
> src/tests/protobuf_io_tests.cpp b6478b3140aee69741a3e62485260f3661835c41
> src/tests/reaper_tests.cpp 0809c1ff17eb949beb1bdd922fdced022aa202f3
> src/tests/resource_offers_tests.cpp 5e53a89bcf99dedd4535545dc44a1a6229a09b0f
> src/tests/script.cpp d7f103eb08335638bca57ccab77646534342b058
> src/tests/slave_recovery_tests.cpp d963ce1251ca28a832ca5d3e00c40aa7911ccb03
> src/tests/state_tests.cpp eb124fdc10491f085007b5a7e9fd47e006f1523f
> src/tests/status_update_manager_tests.cpp e375898152de357b1b50b7c5185b825e51bfaa3e
> src/tests/utils.hpp ca3ecd7f0cab283327bf83e57d4b405b4ada9c74
> src/tests/utils.cpp 9d1d5ad1e192b0f44a7a7173839b292f6bedad15
> src/tests/zookeeper.hpp PRE-CREATION
> src/tests/zookeeper.cpp PRE-CREATION
> src/tests/zookeeper_tests.cpp e9180bfd35ee4477849d3a776c29eff5206d66e9
> src/zookeeper/authentication.hpp fb0e3ca47f0a0327d6358b9e175e1a0452110bf7
> src/zookeeper/url.hpp b297a16e0da54892bf7dd79bbe077f70e1dc9c06
>
> Diff: https://reviews.apache.org/r/11273/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Refactored MesosTest/MesosClusterTest into a generic
fixture for launching in-memory Mesos clusters and updated all tests
appropriately.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11273/#review20973
-----------------------------------------------------------
Ship it!
src/master/hierarchical_allocator_process.hpp
<https://reviews.apache.org/r/11273/#comment43369>
hmm..why the revert?
src/tests/cgroups_tests.cpp
<https://reviews.apache.org/r/11273/#comment43371>
s/HIERARCHY|ROOT/{HIERARCHY|ROOT}/
src/tests/mesos.hpp
<https://reviews.apache.org/r/11273/#comment43357>
Can you pull this up to match google test's style?
https://code.google.com/p/googletest/source/browse/trunk/samples/sample3_unittest.cc
src/tests/mesos.hpp
<https://reviews.apache.org/r/11273/#comment43358>
Can you add a comment here?
src/tests/mesos.cpp
<https://reviews.apache.org/r/11273/#comment43360>
yup. that would be swell.
src/tests/slave_recovery_tests.cpp
<https://reviews.apache.org/r/11273/#comment43364>
No. Kill this.
src/tests/slave_recovery_tests.cpp
<https://reviews.apache.org/r/11273/#comment43365>
I'm sure I'm forgetting something here, but why can't you make this a member of the fixture and simply call Shutdown() in the TearDown() method.
That way you don't have to repeat these 2 lines in all these tests?
src/tests/slave_recovery_tests.cpp
<https://reviews.apache.org/r/11273/#comment43366>
Aha. I see why you instantiated isolators inside the test now.
I wonder if a restartSlave() primitive inside the test fixture would be better?
src/zookeeper/authentication.hpp
<https://reviews.apache.org/r/11273/#comment43367>
How about
CHECK_EQ(scheme, "digest") << "Unsupported authentication scheme". ?
Note, CHECK_EQ prints 'scheme' for you
src/zookeeper/authentication.hpp
<https://reviews.apache.org/r/11273/#comment43368>
ditto
- Vinod Kone
On May 20, 2013, 10:06 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11273/
> -----------------------------------------------------------
>
> (Updated May 20, 2013, 10:06 p.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Description
> -------
>
> This looks bigger than it actually is. ;)
>
>
> Diffs
> -----
>
> src/Makefile.am 343a7c5288e83d20dc6dd4db5353499199b90cb1
> src/master/hierarchical_allocator_process.hpp 2422fbdc49fd575b615e73891e3cda6675358d2d
> src/tests/allocator_tests.cpp 540c05afa54caafea7c0b49773cf2613c87d1ba5
> src/tests/allocator_zookeeper_tests.cpp 2c7deb1ef8d4349964ffa0bdd7b97ae616be1c43
> src/tests/cgroups_tests.cpp 3734315360fc1fb33a487696123a685aa18c9620
> src/tests/cluster.hpp 682b7d6f06e798b0ae7b676b1cdc9df14dd6f414
> src/tests/configurator_tests.cpp e8ba9362a462fda6774db65bbd633b6b1da36c8c
> src/tests/exception_tests.cpp f405a61a7feb73e96b347d6c8e6fd316bdf6a1aa
> src/tests/fault_tolerance_tests.cpp 68cd5fcff266d102d8edf265776517aa2eef6061
> src/tests/files_tests.cpp 5679ecd3a0e6c8cf7db0e8e97c1c2d2d570caf84
> src/tests/flags_tests.cpp e07dbcc25552351f64d644130ca09ed9b15a988b
> src/tests/gc_tests.cpp 97d2685e0f89e059ad424ecc5887eb18acc9612c
> src/tests/group_tests.cpp PRE-CREATION
> src/tests/isolator.hpp 17dc7b30415be1db984ca6290213840770f465a5
> src/tests/isolator_tests.cpp 435c780f05b5bc78d1dc4e9cbcbac74971607dc9
> src/tests/log_tests.cpp 57fb9db5ef79f5e285eb2624bdfe25375fb8598e
> src/tests/logging_tests.cpp 57eae79724c9952c412ff3c23d36fe9d54356b5b
> src/tests/main.cpp e32ec0cac7b7ab9b31f4e419d0c1426b2c8aeb65
> src/tests/master_detector_tests.cpp 521f0279b165f5d3639788ff6a00b772e6f71b7a
> src/tests/master_tests.cpp 1b062d9ce96ae223389dea5f030f51640511ad52
> src/tests/mesos.hpp PRE-CREATION
> src/tests/mesos.cpp PRE-CREATION
> src/tests/monitor_tests.cpp 9973f98c35415cb72e8f5124bf4d7ff402490a39
> src/tests/paths_tests.cpp 84ad9b385cccecdd3ba61366e67c493926588e23
> src/tests/protobuf_io_tests.cpp b6478b3140aee69741a3e62485260f3661835c41
> src/tests/reaper_tests.cpp 0809c1ff17eb949beb1bdd922fdced022aa202f3
> src/tests/resource_offers_tests.cpp 5e53a89bcf99dedd4535545dc44a1a6229a09b0f
> src/tests/script.cpp d7f103eb08335638bca57ccab77646534342b058
> src/tests/slave_recovery_tests.cpp d963ce1251ca28a832ca5d3e00c40aa7911ccb03
> src/tests/state_tests.cpp eb124fdc10491f085007b5a7e9fd47e006f1523f
> src/tests/status_update_manager_tests.cpp e375898152de357b1b50b7c5185b825e51bfaa3e
> src/tests/utils.hpp ca3ecd7f0cab283327bf83e57d4b405b4ada9c74
> src/tests/utils.cpp 9d1d5ad1e192b0f44a7a7173839b292f6bedad15
> src/tests/zookeeper.hpp PRE-CREATION
> src/tests/zookeeper.cpp PRE-CREATION
> src/tests/zookeeper_tests.cpp e9180bfd35ee4477849d3a776c29eff5206d66e9
> src/zookeeper/authentication.hpp fb0e3ca47f0a0327d6358b9e175e1a0452110bf7
> src/zookeeper/url.hpp b297a16e0da54892bf7dd79bbe077f70e1dc9c06
>
> Diff: https://reviews.apache.org/r/11273/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>