You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Michael Park <mc...@gmail.com> on 2015/03/23 18:21:49 UTC
Re: Review Request 29748: Added tests for Phase 1 of Dynamic
Reservation.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29748/
-----------------------------------------------------------
(Updated March 23, 2015, 5:21 p.m.)
Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
Summary (updated)
-----------------
Added tests for Phase 1 of Dynamic Reservation.
Bugs: MESOS-2489
https://issues.apache.org/jira/browse/MESOS-2489
Repository: mesos
Description
-------
See summary.
Diffs
-----
src/Makefile.am 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb
src/tests/reservation_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/29748/diff/
Testing
-------
make check
Thanks,
Michael Park
Re: Review Request 29748: Added tests for dynamic reservation.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29748/#review81591
-----------------------------------------------------------
src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/29748/#comment132026>
How about a test with non-specified principal?
- Alexander Rukletsov
On April 8, 2015, 5:05 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29748/
> -----------------------------------------------------------
>
> (Updated April 8, 2015, 5:05 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2489
> https://issues.apache.org/jira/browse/MESOS-2489
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am 9c01f5d6c692f835100e7cade928748cc4763cc8
> src/tests/reservation_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/29748/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 29748: Added tests for dynamic reservation.
Posted by Jie Yu <yu...@gmail.com>.
> On April 23, 2015, 1:48 p.m., Michael Park wrote:
> > As of now, the tests seem to take a long time to complete. We should investigate what the issue is before committing this patch.
>
> Jie Yu wrote:
> I suspect this is due to the default allocation interval (1 secs by default).
Ping, has this been resolved?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29748/#review81311
-----------------------------------------------------------
On May 2, 2015, 4:14 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29748/
> -----------------------------------------------------------
>
> (Updated May 2, 2015, 4:14 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2489
> https://issues.apache.org/jira/browse/MESOS-2489
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
> src/tests/reservation_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/29748/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 29748: Added tests for dynamic reservation.
Posted by Jie Yu <yu...@gmail.com>.
> On April 23, 2015, 1:48 p.m., Michael Park wrote:
> > As of now, the tests seem to take a long time to complete. We should investigate what the issue is before committing this patch.
I suspect this is due to the default allocation interval (1 secs by default).
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29748/#review81311
-----------------------------------------------------------
On April 8, 2015, 5:05 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29748/
> -----------------------------------------------------------
>
> (Updated April 8, 2015, 5:05 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2489
> https://issues.apache.org/jira/browse/MESOS-2489
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am 9c01f5d6c692f835100e7cade928748cc4763cc8
> src/tests/reservation_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/29748/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 29748: Added tests for dynamic reservation.
Posted by Michael Park <mc...@gmail.com>.
> On April 23, 2015, 1:48 p.m., Michael Park wrote:
> > As of now, the tests seem to take a long time to complete. We should investigate what the issue is before committing this patch.
>
> Jie Yu wrote:
> I suspect this is due to the default allocation interval (1 secs by default).
>
> Jie Yu wrote:
> Ping, has this been resolved?
>
> Michael Park wrote:
> Not yet. I'll look into this further today.
Resolved this with AlexR offline. Turns out the allocation_interval (default 1s) as well as Filters (default 5s) need to be manually optimized.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29748/#review81311
-----------------------------------------------------------
On May 9, 2015, 4:38 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29748/
> -----------------------------------------------------------
>
> (Updated May 9, 2015, 4:38 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2489
> https://issues.apache.org/jira/browse/MESOS-2489
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
> src/tests/reservation_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/29748/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 29748: Added tests for dynamic reservation.
Posted by Michael Park <mc...@gmail.com>.
> On April 23, 2015, 1:48 p.m., Michael Park wrote:
> > As of now, the tests seem to take a long time to complete. We should investigate what the issue is before committing this patch.
>
> Jie Yu wrote:
> I suspect this is due to the default allocation interval (1 secs by default).
>
> Jie Yu wrote:
> Ping, has this been resolved?
Not yet. I'll look into this further today.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29748/#review81311
-----------------------------------------------------------
On May 2, 2015, 4:14 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29748/
> -----------------------------------------------------------
>
> (Updated May 2, 2015, 4:14 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2489
> https://issues.apache.org/jira/browse/MESOS-2489
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
> src/tests/reservation_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/29748/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 29748: Added tests for dynamic reservation.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29748/#review81311
-----------------------------------------------------------
As of now, the tests seem to take a long time to complete. We should investigate what the issue is before committing this patch.
- Michael Park
On April 8, 2015, 5:05 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29748/
> -----------------------------------------------------------
>
> (Updated April 8, 2015, 5:05 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2489
> https://issues.apache.org/jira/browse/MESOS-2489
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am 9c01f5d6c692f835100e7cade928748cc4763cc8
> src/tests/reservation_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/29748/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 29748: Added tests for dynamic reservation.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29748/
-----------------------------------------------------------
(Updated May 12, 2015, 6:41 p.m.)
Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
Changes
-------
Addressed Jie's comment about redundant tests.
Removed tests:
* `DropReserveNonMatchingRole`
* `DropReserveNonMatchingPrincipal`
* `DropReserveFrameworkMissingPrincipal`
Since they'are already covered in `src/tests/master_validation_tests.cpp`.
Migrated tests:
* `DropReserveAsStaticReservation`
* `DropUnreserveStaticReservation`
to `src/tests/master_validation_tests.cpp` as
* `TEST_F(ReserveOperationValidationTest, StaticReservation)`
* `TEST_F(UnreserveOperationValidationTest, StaticReservation)`
Bugs: MESOS-2489
https://issues.apache.org/jira/browse/MESOS-2489
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693
src/tests/master_validation_tests.cpp 1366bcd229f4cb62df7d181c42dae4152435bb14
src/tests/reservation_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/29748/diff/
Testing
-------
make check
Thanks,
Michael Park
Re: Review Request 29748: Added tests for dynamic reservation.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29748/
-----------------------------------------------------------
(Updated May 12, 2015, 5:13 p.m.)
Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
Changes
-------
Addressed Jie's comments.
Bugs: MESOS-2489
https://issues.apache.org/jira/browse/MESOS-2489
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693
src/tests/reservation_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/29748/diff/
Testing
-------
make check
Thanks,
Michael Park
Re: Review Request 29748: Added tests for dynamic reservation.
Posted by Michael Park <mc...@gmail.com>.
> On May 11, 2015, 11:32 p.m., Jie Yu wrote:
> > src/tests/reservation_tests.cpp, line 365
> > <https://reviews.apache.org/r/29748/diff/17/?file=954334#file954334line365>
> >
> > YOu do need a snake_case checker:)
Sigh... Sorry :( Fixed.
> On May 11, 2015, 11:32 p.m., Jie Yu wrote:
> > src/tests/reservation_tests.cpp, lines 399-400
> > <https://reviews.apache.org/r/29748/diff/17/?file=954334#file954334line399>
> >
> > Hum, have the following 3 tests already been captured in ReserveOperationValidationTest?
Yes, they're captured in the following test cases. What would you like to see done here?
```cpp
TEST_F(ReserveOperationValidationTest, NonMatchingRole)
TEST_F(ReserveOperationValidationTest, NonMatchingPrincipal)
TEST_F(ReserveOperationValidationTest, FrameworkMissingPrincipal)
```
> On May 11, 2015, 11:32 p.m., Jie Yu wrote:
> > src/tests/reservation_tests.cpp, line 944
> > <https://reviews.apache.org/r/29748/diff/17/?file=954334#file954334line944>
> >
> > 2 lines apart between two top level declarations. Please make sure this is the case in this file.
Fixed.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29748/#review83313
-----------------------------------------------------------
On May 12, 2015, 5:13 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29748/
> -----------------------------------------------------------
>
> (Updated May 12, 2015, 5:13 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2489
> https://issues.apache.org/jira/browse/MESOS-2489
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693
> src/tests/reservation_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/29748/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 29748: Added tests for dynamic reservation.
Posted by Jie Yu <yu...@gmail.com>.
> On May 11, 2015, 11:32 p.m., Jie Yu wrote:
> > src/tests/reservation_tests.cpp, lines 399-400
> > <https://reviews.apache.org/r/29748/diff/17/?file=954334#file954334line399>
> >
> > Hum, have the following 3 tests already been captured in ReserveOperationValidationTest?
>
> Michael Park wrote:
> Yes, they're captured in the following test cases. What would you like to see done here?
>
> ```cpp
> TEST_F(ReserveOperationValidationTest, NonMatchingRole)
> TEST_F(ReserveOperationValidationTest, NonMatchingPrincipal)
> TEST_F(ReserveOperationValidationTest, FrameworkMissingPrincipal)
> ```
I would rather remove just remove these three tests here. Seems to be pretty redundent.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29748/#review83313
-----------------------------------------------------------
On May 12, 2015, 5:13 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29748/
> -----------------------------------------------------------
>
> (Updated May 12, 2015, 5:13 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2489
> https://issues.apache.org/jira/browse/MESOS-2489
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693
> src/tests/reservation_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/29748/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 29748: Added tests for dynamic reservation.
Posted by Michael Park <mc...@gmail.com>.
> On May 11, 2015, 11:32 p.m., Jie Yu wrote:
> > src/tests/reservation_tests.cpp, lines 399-400
> > <https://reviews.apache.org/r/29748/diff/17/?file=954334#file954334line399>
> >
> > Hum, have the following 3 tests already been captured in ReserveOperationValidationTest?
>
> Michael Park wrote:
> Yes, they're captured in the following test cases. What would you like to see done here?
>
> ```cpp
> TEST_F(ReserveOperationValidationTest, NonMatchingRole)
> TEST_F(ReserveOperationValidationTest, NonMatchingPrincipal)
> TEST_F(ReserveOperationValidationTest, FrameworkMissingPrincipal)
> ```
>
> Jie Yu wrote:
> I would rather remove just remove these three tests here. Seems to be pretty redundent.
Ok, I've removed the 3 tests you mentioned and migrated 2 tests that can be done as master validation to `src/tests/master_validation_tests.cpp`.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29748/#review83313
-----------------------------------------------------------
On May 12, 2015, 6:41 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29748/
> -----------------------------------------------------------
>
> (Updated May 12, 2015, 6:41 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2489
> https://issues.apache.org/jira/browse/MESOS-2489
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693
> src/tests/master_validation_tests.cpp 1366bcd229f4cb62df7d181c42dae4152435bb14
> src/tests/reservation_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/29748/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 29748: Added tests for dynamic reservation.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29748/#review83313
-----------------------------------------------------------
Ship it!
src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/29748/#comment134249>
YOu do need a snake_case checker:)
src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/29748/#comment134251>
Hum, have the following 3 tests already been captured in ReserveOperationValidationTest?
src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/29748/#comment134256>
2 lines apart between two top level declarations. Please make sure this is the case in this file.
- Jie Yu
On May 9, 2015, 4:38 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29748/
> -----------------------------------------------------------
>
> (Updated May 9, 2015, 4:38 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2489
> https://issues.apache.org/jira/browse/MESOS-2489
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
> src/tests/reservation_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/29748/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 29748: Added tests for dynamic reservation.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29748/
-----------------------------------------------------------
(Updated May 9, 2015, 4:38 a.m.)
Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
Changes
-------
* Updated to explicitly use the `TestAllocator` only in cases where it's needed.
* Added `allocation_interval` and `Filters` optimizations to speed up tests to an acceptable state.
* Added new tests.
* A framework without a principal attempting to reserve resources.
* Attempting to dynamically reserve statically reserved resources.
* Attempting to dynamically reserve resources into statically reserved state.
* Attempting to unreserve statically reserved resources.
Bugs: MESOS-2489
https://issues.apache.org/jira/browse/MESOS-2489
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
src/tests/reservation_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/29748/diff/
Testing
-------
make check
Thanks,
Michael Park
Re: Review Request 29748: Added tests for dynamic reservation.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29748/
-----------------------------------------------------------
(Updated May 2, 2015, 4:14 a.m.)
Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
Bugs: MESOS-2489
https://issues.apache.org/jira/browse/MESOS-2489
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
src/tests/reservation_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/29748/diff/
Testing
-------
make check
Thanks,
Michael Park
Re: Review Request 29748: Added tests for dynamic reservation.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29748/
-----------------------------------------------------------
(Updated May 2, 2015, 4 a.m.)
Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
Changes
-------
Moved the declarations of resources up and added more comments.
Bugs: MESOS-2489
https://issues.apache.org/jira/browse/MESOS-2489
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
src/tests/reservation_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/29748/diff/
Testing
-------
make check
Thanks,
Michael Park
Re: Review Request 29748: Added tests for dynamic reservation.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29748/
-----------------------------------------------------------
(Updated May 2, 2015, 1:32 a.m.)
Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
Bugs: MESOS-2489
https://issues.apache.org/jira/browse/MESOS-2489
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
src/tests/reservation_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/29748/diff/
Testing
-------
make check
Thanks,
Michael Park
Re: Review Request 29748: Added tests for dynamic reservation.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29748/
-----------------------------------------------------------
(Updated May 2, 2015, 1:29 a.m.)
Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
Changes
-------
Addressed some of AlexR's comments.
Bugs: MESOS-2489
https://issues.apache.org/jira/browse/MESOS-2489
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
src/tests/reservation_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/29748/diff/
Testing
-------
make check
Thanks,
Michael Park
Re: Review Request 29748: Added tests for dynamic reservation.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29748/
-----------------------------------------------------------
(Updated May 2, 2015, 12:49 a.m.)
Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
Changes
-------
Added a tab to line up the `\`.
Bugs: MESOS-2489
https://issues.apache.org/jira/browse/MESOS-2489
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
src/tests/reservation_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/29748/diff/
Testing
-------
make check
Thanks,
Michael Park
Re: Review Request 29748: Added tests for dynamic reservation.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On April 23, 2015, 4:15 p.m., Alexander Rukletsov wrote:
> > src/tests/reservation_tests.cpp, line 108
> > <https://reviews.apache.org/r/29748/diff/11/?file=920958#file920958line108>
> >
> > How about we use a single resource string for clarity? Here we start a slave with `"cpus:1;mem:512;disk:0;ports:[]"` resources, while expect `"cpus:1;mem:512"` in the offer.
>
> Michael Park wrote:
> The slave resources are specified as `cpus:1;mem:512;disk:0;ports:[]` because we use `Containerizer::resources` to parse the string which tries to prove the OS for a value if unspecified. In these test cases we try to reserve all of the resources, but we'll also have tests that don't reserve all of the resources. I thought it would be good to keep the slave resources and the reserved resources separated. What do you think?
>
> Michael Park wrote:
> `s/prove/probe`
I see, that makes sense! Mind transforming and leaving this as a short note in the code and then fix the issue?
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29748/#review81298
-----------------------------------------------------------
On May 2, 2015, 4:14 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29748/
> -----------------------------------------------------------
>
> (Updated May 2, 2015, 4:14 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2489
> https://issues.apache.org/jira/browse/MESOS-2489
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
> src/tests/reservation_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/29748/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 29748: Added tests for dynamic reservation.
Posted by Michael Park <mc...@gmail.com>.
> On April 23, 2015, 4:15 p.m., Alexander Rukletsov wrote:
> > src/tests/reservation_tests.cpp, line 108
> > <https://reviews.apache.org/r/29748/diff/11/?file=920958#file920958line108>
> >
> > How about we use a single resource string for clarity? Here we start a slave with `"cpus:1;mem:512;disk:0;ports:[]"` resources, while expect `"cpus:1;mem:512"` in the offer.
>
> Michael Park wrote:
> The slave resources are specified as `cpus:1;mem:512;disk:0;ports:[]` because we use `Containerizer::resources` to parse the string which tries to prove the OS for a value if unspecified. In these test cases we try to reserve all of the resources, but we'll also have tests that don't reserve all of the resources. I thought it would be good to keep the slave resources and the reserved resources separated. What do you think?
`s/prove/probe`
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29748/#review81298
-----------------------------------------------------------
On May 2, 2015, 4:14 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29748/
> -----------------------------------------------------------
>
> (Updated May 2, 2015, 4:14 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2489
> https://issues.apache.org/jira/browse/MESOS-2489
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
> src/tests/reservation_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/29748/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 29748: Added tests for dynamic reservation.
Posted by Michael Park <mc...@gmail.com>.
> On April 23, 2015, 4:15 p.m., Alexander Rukletsov wrote:
> > src/tests/reservation_tests.cpp, line 108
> > <https://reviews.apache.org/r/29748/diff/11/?file=920958#file920958line108>
> >
> > How about we use a single resource string for clarity? Here we start a slave with `"cpus:1;mem:512;disk:0;ports:[]"` resources, while expect `"cpus:1;mem:512"` in the offer.
>
> Michael Park wrote:
> The slave resources are specified as `cpus:1;mem:512;disk:0;ports:[]` because we use `Containerizer::resources` to parse the string which tries to prove the OS for a value if unspecified. In these test cases we try to reserve all of the resources, but we'll also have tests that don't reserve all of the resources. I thought it would be good to keep the slave resources and the reserved resources separated. What do you think?
>
> Michael Park wrote:
> `s/prove/probe`
>
> Alexander Rukletsov wrote:
> I see, that makes sense! Mind transforming and leaving this as a short note in the code and then fix the issue?
I've changed the pattern such that the slave resources are also specified as `cpus:1;mem:512`, but we check for subset rather than equality.
```cpp
EXPECT_TRUE(Resources(offer.resources()).contains(unreserved));
```
> On April 23, 2015, 4:15 p.m., Alexander Rukletsov wrote:
> > src/tests/reservation_tests.cpp, line 148
> > <https://reviews.apache.org/r/29748/diff/11/?file=920958#file920958line148>
> >
> > The default for `Flags::allocation_interval` is 1s, but I hope the test is faster, right? Why do we get the next offer earlier than after 1s?
Resolved this with AlexR offline. Turns out the `allocation_interval` (default 1s) as well as `Filters` (default 5s) need to be manually optimized.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29748/#review81298
-----------------------------------------------------------
On May 9, 2015, 4:38 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29748/
> -----------------------------------------------------------
>
> (Updated May 9, 2015, 4:38 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2489
> https://issues.apache.org/jira/browse/MESOS-2489
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
> src/tests/reservation_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/29748/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 29748: Added tests for dynamic reservation.
Posted by Michael Park <mc...@gmail.com>.
> On April 23, 2015, 4:15 p.m., Alexander Rukletsov wrote:
> > src/tests/reservation_tests.cpp, line 108
> > <https://reviews.apache.org/r/29748/diff/11/?file=920958#file920958line108>
> >
> > How about we use a single resource string for clarity? Here we start a slave with `"cpus:1;mem:512;disk:0;ports:[]"` resources, while expect `"cpus:1;mem:512"` in the offer.
The slave resources are specified as `cpus:1;mem:512;disk:0;ports:[]` because we use `Containerizer::resources` to parse the string which tries to prove the OS for a value if unspecified. In these test cases we try to reserve all of the resources, but we'll also have tests that don't reserve all of the resources. I thought it would be good to keep the slave resources and the reserved resources separated. What do you think?
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29748/#review81298
-----------------------------------------------------------
On May 2, 2015, 4:14 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29748/
> -----------------------------------------------------------
>
> (Updated May 2, 2015, 4:14 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2489
> https://issues.apache.org/jira/browse/MESOS-2489
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
> src/tests/reservation_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/29748/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 29748: Added tests for dynamic reservation.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29748/#review81298
-----------------------------------------------------------
How about a test trying to unreserve statically reserved resources? Let's document the expected behaviour explicitly in the test.
src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/29748/#comment131620>
How about we use a single resource string for clarity? Here we start a slave with `"cpus:1;mem:512;disk:0;ports:[]"` resources, while expect `"cpus:1;mem:512"` in the offer.
src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/29748/#comment131622>
The default for `Flags::allocation_interval` is 1s, but I hope the test is faster, right? Why do we get the next offer earlier than after 1s?
src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/29748/#comment131631>
... and decline it.
src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/29748/#comment131640>
Isn't setting this param to 0 tells master that framework1 can be offered the same resource immediately? If you want framework2 to get the offer, this looks flaky.
src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/29748/#comment131648>
... and reserve it for a different role.
src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/29748/#comment131649>
I'm not sure what is the preferred way of doing this in our codebase, but I would rather prefer having "role1" and "role2" defined in the beginning of the test with descriptive names, e.g.
```
const std::string frameworkRole = "role1";
const std::string reservationRole = "role2";
```
src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/29748/#comment131654>
Mind leaving a comment here or above why updateAllocation should not be called? Like you do in the next test.
src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/29748/#comment131650>
// In the next offer, still expect an offer with the unreserved resources.
src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/29748/#comment131651>
Mind leaving a comment that we have a default credential set there and it is different to the one we are going to use to reserve?
src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/29748/#comment131652>
Again, let's move it to the beginning, closer to `DEFAULT_FRAMEWORK_INFO`.
src/tests/reservation_tests.cpp
<https://reviews.apache.org/r/29748/#comment131653>
Mind leaving a comment here or above why updateAllocation should not be called? Like you do in the next test.
- Alexander Rukletsov
On April 8, 2015, 5:05 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29748/
> -----------------------------------------------------------
>
> (Updated April 8, 2015, 5:05 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
>
>
> Bugs: MESOS-2489
> https://issues.apache.org/jira/browse/MESOS-2489
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am 9c01f5d6c692f835100e7cade928748cc4763cc8
> src/tests/reservation_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/29748/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 29748: Added tests for dynamic reservation.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29748/
-----------------------------------------------------------
(Updated April 8, 2015, 5:05 p.m.)
Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
Bugs: MESOS-2489
https://issues.apache.org/jira/browse/MESOS-2489
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/Makefile.am 9c01f5d6c692f835100e7cade928748cc4763cc8
src/tests/reservation_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/29748/diff/
Testing
-------
make check
Thanks,
Michael Park
Re: Review Request 29748: Added tests for dynamic reservation.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29748/
-----------------------------------------------------------
(Updated April 7, 2015, 9:44 p.m.)
Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
Summary (updated)
-----------------
Added tests for dynamic reservation.
Bugs: MESOS-2489
https://issues.apache.org/jira/browse/MESOS-2489
Repository: mesos
Description
-------
See summary.
Diffs
-----
src/Makefile.am 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb
src/tests/reservation_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/29748/diff/
Testing
-------
make check
Thanks,
Michael Park