You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Greg Mann <gr...@mesosphere.io> on 2015/12/05 01:37:30 UTC
Review Request 41001: Improved
'ReservationTest.ACLMultipleOperations'.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41001/
-----------------------------------------------------------
Review request for mesos.
Repository: mesos
Description
-------
Improved 'ReservationTest.ACLMultipleOperations'.
Diffs
-----
src/tests/reservation_tests.cpp 3fdf5e121840fe99057e917cca48f1425eff6624
Diff: https://reviews.apache.org/r/41001/diff/
Testing
-------
`GTEST_FILTER="ReservationTest.ACLMultipleOperations" bin/mesos-tests.sh --gtest_repeat=1000 --gtest_break_on_failure`
This test was originally written to test the interaction of multiple RESERVE/UNRESERVE/LAUNCH operations in a single `acceptOffers()` call when authorization is enabled. In order to probe the effect of authorization on this interaction, some operations should fail due to failed authorization. However, this test included operations that failed simply because they were malformed. I altered the test so that now operations will fail due to failed authorization, which tests the desired functionality in a more thorough way.
Thanks,
Greg Mann
Re: Review Request 41001: Improved
'ReservationTest.ACLMultipleOperations'.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Dec. 9, 2015, 5:56 a.m., Michael Park wrote:
> > src/tests/reservation_tests.cpp, line 1660
> > <https://reviews.apache.org/r/41001/diff/2/?file=1156013#file1156013line1660>
> >
> > This one gets dropped because it's an invalid unreserve request, right? This is still what's intended to be tested?
Ah sorry, I intended to make this a valid request. Fixed.
> On Dec. 9, 2015, 5:56 a.m., Michael Park wrote:
> > src/tests/reservation_tests.cpp, lines 1703-1704
> > <https://reviews.apache.org/r/41001/diff/2/?file=1156013#file1156013line1703>
> >
> > Just to make sure I understand the intent of this patch, this is the part where the operations are dropped due to authorization rather than invalid request. Correct?
Yep, that's correct. In `master.cpp`, `accept()` passes a couple lists along to its continuation: one of them is a list of authorization results, another is a list of operations, and they are iterated through at the same time (the authorization at a given index corresponds to the operation at the same index). Performing both successful and failed RESERVE/UNRESERVE operations, along with an always-successful LAUNCH operation, tests that these authorization results are being passed to the continuation, and in the correct order.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41001/#review109458
-----------------------------------------------------------
On Dec. 10, 2015, 2:02 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41001/
> -----------------------------------------------------------
>
> (Updated Dec. 10, 2015, 2:02 a.m.)
>
>
> Review request for mesos, Jie Yu and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Improved 'ReservationTest.ACLMultipleOperations'.
>
>
> Diffs
> -----
>
> src/tests/reservation_tests.cpp 3405b47c367e3c20e2a9139ab95dd8fd1805ea8d
>
> Diff: https://reviews.apache.org/r/41001/diff/
>
>
> Testing
> -------
>
> `GTEST_FILTER="ReservationTest.ACLMultipleOperations" bin/mesos-tests.sh --gtest_repeat=1000 --gtest_break_on_failure`
>
> This test was originally written to test the interaction of multiple RESERVE/UNRESERVE/LAUNCH operations in a single `acceptOffers()` call when authorization is enabled. In order to probe the effect of authorization on this interaction, some operations should fail due to failed authorization. However, this test included operations that failed simply because they were malformed. I altered the test so that now operations will fail due to failed authorization, which tests the desired functionality more precisely.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 41001: Improved
'ReservationTest.ACLMultipleOperations'.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41001/#review109458
-----------------------------------------------------------
src/tests/reservation_tests.cpp (line 1548)
<https://reviews.apache.org/r/41001/#comment169000>
`s/can/cannot/`?
src/tests/reservation_tests.cpp (line 1660)
<https://reviews.apache.org/r/41001/#comment169003>
This one gets dropped because it's an invalid unreserve request, right? This is still what's intended to be tested?
src/tests/reservation_tests.cpp (lines 1690 - 1691)
<https://reviews.apache.org/r/41001/#comment169001>
Rather than changing `taskInfo` midway through the test, can we just label them `taskInfo1` and `taskInfo2` corresponding to `dynamicallyReserved1` and `dynamicallyReserved2` respectively?
src/tests/reservation_tests.cpp (lines 1703 - 1704)
<https://reviews.apache.org/r/41001/#comment169005>
Just to make sure I understand the intent of this patch, this is the part where the operations are dropped due to authorization rather than invalid request. Correct?
- Michael Park
On Dec. 8, 2015, 5:14 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41001/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2015, 5:14 p.m.)
>
>
> Review request for mesos, Jie Yu and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Improved 'ReservationTest.ACLMultipleOperations'.
>
>
> Diffs
> -----
>
> src/tests/reservation_tests.cpp eccbb8f8a02b65b26f34e020e736afe0445a6d0d
>
> Diff: https://reviews.apache.org/r/41001/diff/
>
>
> Testing
> -------
>
> `GTEST_FILTER="ReservationTest.ACLMultipleOperations" bin/mesos-tests.sh --gtest_repeat=1000 --gtest_break_on_failure`
>
> This test was originally written to test the interaction of multiple RESERVE/UNRESERVE/LAUNCH operations in a single `acceptOffers()` call when authorization is enabled. In order to probe the effect of authorization on this interaction, some operations should fail due to failed authorization. However, this test included operations that failed simply because they were malformed. I altered the test so that now operations will fail due to failed authorization, which tests the desired functionality more precisely.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 41001: Improved
'ReservationTest.ACLMultipleOperations'.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41001/#review109735
-----------------------------------------------------------
Patch looks great!
Reviews applied: [40999, 41001]
Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
- Mesos ReviewBot
On Dec. 10, 2015, 2:02 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41001/
> -----------------------------------------------------------
>
> (Updated Dec. 10, 2015, 2:02 a.m.)
>
>
> Review request for mesos, Jie Yu and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Improved 'ReservationTest.ACLMultipleOperations'.
>
>
> Diffs
> -----
>
> src/tests/reservation_tests.cpp 3405b47c367e3c20e2a9139ab95dd8fd1805ea8d
>
> Diff: https://reviews.apache.org/r/41001/diff/
>
>
> Testing
> -------
>
> `GTEST_FILTER="ReservationTest.ACLMultipleOperations" bin/mesos-tests.sh --gtest_repeat=1000 --gtest_break_on_failure`
>
> This test was originally written to test the interaction of multiple RESERVE/UNRESERVE/LAUNCH operations in a single `acceptOffers()` call when authorization is enabled. In order to probe the effect of authorization on this interaction, some operations should fail due to failed authorization. However, this test included operations that failed simply because they were malformed. I altered the test so that now operations will fail due to failed authorization, which tests the desired functionality more precisely.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 41001: Improved
'ReservationTest.ACLMultipleOperations'.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41001/#review109732
-----------------------------------------------------------
Ship it!
Ship It!
- Michael Park
On Dec. 10, 2015, 2:02 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41001/
> -----------------------------------------------------------
>
> (Updated Dec. 10, 2015, 2:02 a.m.)
>
>
> Review request for mesos, Jie Yu and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Improved 'ReservationTest.ACLMultipleOperations'.
>
>
> Diffs
> -----
>
> src/tests/reservation_tests.cpp 3405b47c367e3c20e2a9139ab95dd8fd1805ea8d
>
> Diff: https://reviews.apache.org/r/41001/diff/
>
>
> Testing
> -------
>
> `GTEST_FILTER="ReservationTest.ACLMultipleOperations" bin/mesos-tests.sh --gtest_repeat=1000 --gtest_break_on_failure`
>
> This test was originally written to test the interaction of multiple RESERVE/UNRESERVE/LAUNCH operations in a single `acceptOffers()` call when authorization is enabled. In order to probe the effect of authorization on this interaction, some operations should fail due to failed authorization. However, this test included operations that failed simply because they were malformed. I altered the test so that now operations will fail due to failed authorization, which tests the desired functionality more precisely.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 41001: Improved
'ReservationTest.ACLMultipleOperations'.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41001/
-----------------------------------------------------------
(Updated Dec. 10, 2015, 2:02 a.m.)
Review request for mesos, Jie Yu and Michael Park.
Changes
-------
Addressed comment.
Repository: mesos
Description
-------
Improved 'ReservationTest.ACLMultipleOperations'.
Diffs (updated)
-----
src/tests/reservation_tests.cpp 3405b47c367e3c20e2a9139ab95dd8fd1805ea8d
Diff: https://reviews.apache.org/r/41001/diff/
Testing
-------
`GTEST_FILTER="ReservationTest.ACLMultipleOperations" bin/mesos-tests.sh --gtest_repeat=1000 --gtest_break_on_failure`
This test was originally written to test the interaction of multiple RESERVE/UNRESERVE/LAUNCH operations in a single `acceptOffers()` call when authorization is enabled. In order to probe the effect of authorization on this interaction, some operations should fail due to failed authorization. However, this test included operations that failed simply because they were malformed. I altered the test so that now operations will fail due to failed authorization, which tests the desired functionality more precisely.
Thanks,
Greg Mann
Re: Review Request 41001: Improved
'ReservationTest.ACLMultipleOperations'.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41001/
-----------------------------------------------------------
(Updated Dec. 10, 2015, 1:18 a.m.)
Review request for mesos, Jie Yu and Michael Park.
Changes
-------
Addressed comments.
Repository: mesos
Description
-------
Improved 'ReservationTest.ACLMultipleOperations'.
Diffs (updated)
-----
src/tests/reservation_tests.cpp 3405b47c367e3c20e2a9139ab95dd8fd1805ea8d
Diff: https://reviews.apache.org/r/41001/diff/
Testing
-------
`GTEST_FILTER="ReservationTest.ACLMultipleOperations" bin/mesos-tests.sh --gtest_repeat=1000 --gtest_break_on_failure`
This test was originally written to test the interaction of multiple RESERVE/UNRESERVE/LAUNCH operations in a single `acceptOffers()` call when authorization is enabled. In order to probe the effect of authorization on this interaction, some operations should fail due to failed authorization. However, this test included operations that failed simply because they were malformed. I altered the test so that now operations will fail due to failed authorization, which tests the desired functionality more precisely.
Thanks,
Greg Mann
Re: Review Request 41001: Improved
'ReservationTest.ACLMultipleOperations'.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41001/#review109372
-----------------------------------------------------------
Patch looks great!
Reviews applied: [40999, 41001]
Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
- Mesos ReviewBot
On Dec. 8, 2015, 5:14 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41001/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2015, 5:14 p.m.)
>
>
> Review request for mesos, Jie Yu and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Improved 'ReservationTest.ACLMultipleOperations'.
>
>
> Diffs
> -----
>
> src/tests/reservation_tests.cpp eccbb8f8a02b65b26f34e020e736afe0445a6d0d
>
> Diff: https://reviews.apache.org/r/41001/diff/
>
>
> Testing
> -------
>
> `GTEST_FILTER="ReservationTest.ACLMultipleOperations" bin/mesos-tests.sh --gtest_repeat=1000 --gtest_break_on_failure`
>
> This test was originally written to test the interaction of multiple RESERVE/UNRESERVE/LAUNCH operations in a single `acceptOffers()` call when authorization is enabled. In order to probe the effect of authorization on this interaction, some operations should fail due to failed authorization. However, this test included operations that failed simply because they were malformed. I altered the test so that now operations will fail due to failed authorization, which tests the desired functionality more precisely.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 41001: Improved
'ReservationTest.ACLMultipleOperations'.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41001/
-----------------------------------------------------------
(Updated Dec. 8, 2015, 5:14 p.m.)
Review request for mesos, Jie Yu and Michael Park.
Changes
-------
Rebase.
Repository: mesos
Description
-------
Improved 'ReservationTest.ACLMultipleOperations'.
Diffs (updated)
-----
src/tests/reservation_tests.cpp eccbb8f8a02b65b26f34e020e736afe0445a6d0d
Diff: https://reviews.apache.org/r/41001/diff/
Testing (updated)
-------
`GTEST_FILTER="ReservationTest.ACLMultipleOperations" bin/mesos-tests.sh --gtest_repeat=1000 --gtest_break_on_failure`
This test was originally written to test the interaction of multiple RESERVE/UNRESERVE/LAUNCH operations in a single `acceptOffers()` call when authorization is enabled. In order to probe the effect of authorization on this interaction, some operations should fail due to failed authorization. However, this test included operations that failed simply because they were malformed. I altered the test so that now operations will fail due to failed authorization, which tests the desired functionality more precisely.
Thanks,
Greg Mann
Re: Review Request 41001: Improved
'ReservationTest.ACLMultipleOperations'.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41001/#review109081
-----------------------------------------------------------
Bad review!
Reviews applied: []
Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing list.
- Mesos ReviewBot
On Dec. 5, 2015, 12:37 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41001/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2015, 12:37 a.m.)
>
>
> Review request for mesos.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Improved 'ReservationTest.ACLMultipleOperations'.
>
>
> Diffs
> -----
>
> src/tests/reservation_tests.cpp 3fdf5e121840fe99057e917cca48f1425eff6624
>
> Diff: https://reviews.apache.org/r/41001/diff/
>
>
> Testing
> -------
>
> `GTEST_FILTER="ReservationTest.ACLMultipleOperations" bin/mesos-tests.sh --gtest_repeat=1000 --gtest_break_on_failure`
>
> This test was originally written to test the interaction of multiple RESERVE/UNRESERVE/LAUNCH operations in a single `acceptOffers()` call when authorization is enabled. In order to probe the effect of authorization on this interaction, some operations should fail due to failed authorization. However, this test included operations that failed simply because they were malformed. I altered the test so that now operations will fail due to failed authorization, which tests the desired functionality in a more thorough way.
>
>
> Thanks,
>
> Greg Mann
>
>