You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Joseph Wu <jo...@mesosphere.io> on 2016/01/09 02:58:03 UTC
Re: Review Request 42086: Updated and refactored Master::accept for
Offers with InverseOffers.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42086/
-----------------------------------------------------------
(Updated Jan. 8, 2016, 5:58 p.m.)
Review request for mesos, Ben Mahler, Artem Harutyunyan, and Joris Van Remoortere.
Changes
-------
Re-implemented the top of `Master::accept` based on what we (Joris/BenM/I) discussed offline. This splits apart offers and inverse offers into separate arrays for validation and processing.
Summary (updated)
-----------------
Updated and refactored Master::accept for Offers with InverseOffers.
Bugs: MESOS-4301
https://issues.apache.org/jira/browse/MESOS-4301
Repository: mesos
Description (updated)
-------
Adds `validation::offer::inverse::validate` to validate inverse offers along the same lines as `validation::offer::validate`, except `SlaveId` is not validated for inverse offers.
Fixes and refactors `Master::accept` to allow `ACCEPT` calls that contain both offers and inverse offers.
Also tweaks `Master::accept` to not print a misleading log line "ACCEPT call used invalid offers ..." when the call only includes inverse offers.
Diffs (updated)
-----
include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a
src/common/type_utils.cpp 76f48f6a1f5467db032ded8acd296d03353b4172
src/master/master.hpp f02d165874fa8023675e545793de699aeecae29b
src/master/master.cpp 2d9b7f9540574aa3ef9e5af3b2b8922dffeebac8
src/master/validation.hpp 380b40279faf180a6f401a5e28280b601dbc648c
src/master/validation.cpp 6a43bce5b7df6a9d939245c4726d060fa19eb305
Diff: https://reviews.apache.org/r/42086/diff/
Testing
-------
make check
# Ran the following and checked for blank output:
bin/mesos-tests.sh --gtest_filter="*Inverse*Offer*" --verbose 2>&1 /dev/null | grep "ACCEPT call used invalid offers"
# Check new test for flakiness:
bin/mesos-tests.sh --gtest_filter="*OffersAndInverseOffers" --gtest_repeat=1500 --gtest_break_on_failure
Thanks,
Joseph Wu
Re: Review Request 42086: Updated and refactored Master::accept for
Offers with InverseOffers.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Jan. 12, 2016, 6:43 a.m., Benjamin Bannier wrote:
> > src/master/master.cpp, line 3084
> > <https://reviews.apache.org/r/42086/diff/3/?file=1192385#file1192385line3084>
> >
> > Since there's a clean separation among offers and inverse offers, it seems we could begin logging unknown offers (probably: at some low prio).
>
> Joseph Wu wrote:
> Each of the validation functions already perform this check. But the error message is `"Offer " + stringify(offerId) + " is no longer valid"`, presumably because re-accepting an offer is more likely than accepting a random unknown `OfferID`.
> This error is, in my diff, printed right after the validation function returns an error.
>
> In light of this, do you think it's still necessary to print each unknown offer?
>
> Benjamin Bannier wrote:
> True, the validation might return an Error if the offer is no longer valid. I am just wondering about the invariants here:
>
> * if an offer could go away concurrently here (probably not) we should still emit an error, e.g., to a log
> * if the offer will always be valid we should just `CHECK` to be explicit.
>
> Does that make sense?
>
> Joseph Wu wrote:
> For each of your points:
>
> * Master, being a libprocess actor, will not remove offers racily in the middle of this function. So if `offers::validate` doesn't error, all the checked offers will exist.
> * If we wanted to be super explicit, we'd end up with some not-necessarily easier-to-read code. For example, to properly implement a `CHECK` (for just regular offers):
> ```
> vector<OfferID> offerIds; // Assume this is filled in...
>
> const Option<Error> offerError = validation::offer::validate(offerIds, this, framework);
>
> if (offerError.isSome()) {
> LOG(WARNING) << "ACCEPT call used invalid offers '" << offerIds << "': " << offerError.get().message;
>
> // We still need to recover the invalid offers, so we need to do this at some point.
> foreach (const OfferID& offerId, offerIds) {
> Offer* offer = getOffer(offerId);
> if (offer != NULL) {
> allocator->recoverResources(
> offer->framework_id(),
> offer->slave_id(),
> offer->resources(),
> None());
>
> removeOffer(offer);
> }
> }
> } else {
> Resources offeredResources;
> Option<SlaveID> slaveId = None();
>
> // Compute offered resources and remove the offers.
> foreach (const OfferID& offerId, offerIds) {
> Offer* offer = getOffer(offerId);
>
> // Here's the check! Since `offerError` is `None()`, all the offers should exist.
> CHECK(offer != NULL);
>
> slaveId = offer->slave_id();
> offeredResources += offer->resources();
>
> removeOffer(offer);
> }
> }
>
> // The rest of the function...
> ```
After double-checking the diff, I could add a `CHECK` for inverse offers, but not for regular offers.
> On Jan. 12, 2016, 6:43 a.m., Benjamin Bannier wrote:
> > src/master/master.cpp, line 3118
> > <https://reviews.apache.org/r/42086/diff/3/?file=1192385#file1192385line3118>
> >
> > Since there's a clean separation among offers and inverse offers, it seems we could begin logging unknown offers (probably: at some low prio).
This will be logged as part of `validation::offer::validate`.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42086/#review113991
-----------------------------------------------------------
On Jan. 12, 2016, 2:20 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42086/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2016, 2:20 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, and Joris Van Remoortere.
>
>
> Bugs: MESOS-4301
> https://issues.apache.org/jira/browse/MESOS-4301
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds `validation::offer::inverse::validate` to validate inverse offers along the same lines as `validation::offer::validate`, except `SlaveId` is not validated for inverse offers.
>
> Fixes and refactors `Master::accept` to allow `ACCEPT` calls that contain both offers and inverse offers.
> Also tweaks `Master::accept` to not print a misleading log line "ACCEPT call used invalid offers ..." when the call only includes inverse offers.
>
>
> Diffs
> -----
>
> include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a
> src/common/type_utils.cpp 76f48f6a1f5467db032ded8acd296d03353b4172
> src/master/master.hpp f02d165874fa8023675e545793de699aeecae29b
> src/master/master.cpp 5268408fc63a28afabc27cba96d3ecb360608a65
> src/master/validation.hpp 380b40279faf180a6f401a5e28280b601dbc648c
> src/master/validation.cpp 6a43bce5b7df6a9d939245c4726d060fa19eb305
>
> Diff: https://reviews.apache.org/r/42086/diff/
>
>
> Testing
> -------
>
> make check
>
> # Ran the following and checked for blank output:
> bin/mesos-tests.sh --gtest_filter="*Inverse*Offer*" --verbose 2>&1 /dev/null | grep "ACCEPT call used invalid offers"
>
> # Check new test for flakiness:
> bin/mesos-tests.sh --gtest_filter="*OffersAndInverseOffers" --gtest_repeat=1500 --gtest_break_on_failure
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 42086: Updated and refactored Master::accept for
Offers with InverseOffers.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Jan. 12, 2016, 6:43 a.m., Benjamin Bannier wrote:
> > src/master/master.cpp, line 3084
> > <https://reviews.apache.org/r/42086/diff/3/?file=1192385#file1192385line3084>
> >
> > Since there's a clean separation among offers and inverse offers, it seems we could begin logging unknown offers (probably: at some low prio).
>
> Joseph Wu wrote:
> Each of the validation functions already perform this check. But the error message is `"Offer " + stringify(offerId) + " is no longer valid"`, presumably because re-accepting an offer is more likely than accepting a random unknown `OfferID`.
> This error is, in my diff, printed right after the validation function returns an error.
>
> In light of this, do you think it's still necessary to print each unknown offer?
>
> Benjamin Bannier wrote:
> True, the validation might return an Error if the offer is no longer valid. I am just wondering about the invariants here:
>
> * if an offer could go away concurrently here (probably not) we should still emit an error, e.g., to a log
> * if the offer will always be valid we should just `CHECK` to be explicit.
>
> Does that make sense?
For each of your points:
* Master, being a libprocess actor, will not remove offers racily in the middle of this function. So if `offers::validate` doesn't error, all the checked offers will exist.
* If we wanted to be super explicit, we'd end up with some not-necessarily easier-to-read code. For example, to properly implement a `CHECK` (for just regular offers):
```
vector<OfferID> offerIds; // Assume this is filled in...
const Option<Error> offerError = validation::offer::validate(offerIds, this, framework);
if (offerError.isSome()) {
LOG(WARNING) << "ACCEPT call used invalid offers '" << offerIds << "': " << offerError.get().message;
// We still need to recover the invalid offers, so we need to do this at some point.
foreach (const OfferID& offerId, offerIds) {
Offer* offer = getOffer(offerId);
if (offer != NULL) {
allocator->recoverResources(
offer->framework_id(),
offer->slave_id(),
offer->resources(),
None());
removeOffer(offer);
}
}
} else {
Resources offeredResources;
Option<SlaveID> slaveId = None();
// Compute offered resources and remove the offers.
foreach (const OfferID& offerId, offerIds) {
Offer* offer = getOffer(offerId);
// Here's the check! Since `offerError` is `None()`, all the offers should exist.
CHECK(offer != NULL);
slaveId = offer->slave_id();
offeredResources += offer->resources();
removeOffer(offer);
}
}
// The rest of the function...
```
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42086/#review113991
-----------------------------------------------------------
On Jan. 11, 2016, 1:31 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42086/
> -----------------------------------------------------------
>
> (Updated Jan. 11, 2016, 1:31 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, and Joris Van Remoortere.
>
>
> Bugs: MESOS-4301
> https://issues.apache.org/jira/browse/MESOS-4301
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds `validation::offer::inverse::validate` to validate inverse offers along the same lines as `validation::offer::validate`, except `SlaveId` is not validated for inverse offers.
>
> Fixes and refactors `Master::accept` to allow `ACCEPT` calls that contain both offers and inverse offers.
> Also tweaks `Master::accept` to not print a misleading log line "ACCEPT call used invalid offers ..." when the call only includes inverse offers.
>
>
> Diffs
> -----
>
> include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a
> src/common/type_utils.cpp 76f48f6a1f5467db032ded8acd296d03353b4172
> src/master/master.hpp f02d165874fa8023675e545793de699aeecae29b
> src/master/master.cpp 2d9b7f9540574aa3ef9e5af3b2b8922dffeebac8
> src/master/validation.hpp 380b40279faf180a6f401a5e28280b601dbc648c
> src/master/validation.cpp 6a43bce5b7df6a9d939245c4726d060fa19eb305
>
> Diff: https://reviews.apache.org/r/42086/diff/
>
>
> Testing
> -------
>
> make check
>
> # Ran the following and checked for blank output:
> bin/mesos-tests.sh --gtest_filter="*Inverse*Offer*" --verbose 2>&1 /dev/null | grep "ACCEPT call used invalid offers"
>
> # Check new test for flakiness:
> bin/mesos-tests.sh --gtest_filter="*OffersAndInverseOffers" --gtest_repeat=1500 --gtest_break_on_failure
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 42086: Updated and refactored Master::accept for
Offers with InverseOffers.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Jan. 12, 2016, 6:43 a.m., Benjamin Bannier wrote:
> > src/master/master.cpp, line 3084
> > <https://reviews.apache.org/r/42086/diff/3/?file=1192385#file1192385line3084>
> >
> > Since there's a clean separation among offers and inverse offers, it seems we could begin logging unknown offers (probably: at some low prio).
Each of the validation functions already perform this check. But the error message is `"Offer " + stringify(offerId) + " is no longer valid"`, presumably because re-accepting an offer is more likely than accepting a random unknown `OfferID`.
This error is, in my diff, printed right after the validation function returns an error.
In light of this, do you think it's still necessary to print each unknown offer?
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42086/#review113991
-----------------------------------------------------------
On Jan. 11, 2016, 1:31 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42086/
> -----------------------------------------------------------
>
> (Updated Jan. 11, 2016, 1:31 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, and Joris Van Remoortere.
>
>
> Bugs: MESOS-4301
> https://issues.apache.org/jira/browse/MESOS-4301
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds `validation::offer::inverse::validate` to validate inverse offers along the same lines as `validation::offer::validate`, except `SlaveId` is not validated for inverse offers.
>
> Fixes and refactors `Master::accept` to allow `ACCEPT` calls that contain both offers and inverse offers.
> Also tweaks `Master::accept` to not print a misleading log line "ACCEPT call used invalid offers ..." when the call only includes inverse offers.
>
>
> Diffs
> -----
>
> include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a
> src/common/type_utils.cpp 76f48f6a1f5467db032ded8acd296d03353b4172
> src/master/master.hpp f02d165874fa8023675e545793de699aeecae29b
> src/master/master.cpp 2d9b7f9540574aa3ef9e5af3b2b8922dffeebac8
> src/master/validation.hpp 380b40279faf180a6f401a5e28280b601dbc648c
> src/master/validation.cpp 6a43bce5b7df6a9d939245c4726d060fa19eb305
>
> Diff: https://reviews.apache.org/r/42086/diff/
>
>
> Testing
> -------
>
> make check
>
> # Ran the following and checked for blank output:
> bin/mesos-tests.sh --gtest_filter="*Inverse*Offer*" --verbose 2>&1 /dev/null | grep "ACCEPT call used invalid offers"
>
> # Check new test for flakiness:
> bin/mesos-tests.sh --gtest_filter="*OffersAndInverseOffers" --gtest_repeat=1500 --gtest_break_on_failure
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 42086: Updated and refactored Master::accept for
Offers with InverseOffers.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Jan. 12, 2016, 3:43 p.m., Benjamin Bannier wrote:
> > src/master/master.cpp, line 3084
> > <https://reviews.apache.org/r/42086/diff/3/?file=1192385#file1192385line3084>
> >
> > Since there's a clean separation among offers and inverse offers, it seems we could begin logging unknown offers (probably: at some low prio).
>
> Joseph Wu wrote:
> Each of the validation functions already perform this check. But the error message is `"Offer " + stringify(offerId) + " is no longer valid"`, presumably because re-accepting an offer is more likely than accepting a random unknown `OfferID`.
> This error is, in my diff, printed right after the validation function returns an error.
>
> In light of this, do you think it's still necessary to print each unknown offer?
True, the validation might return an Error if the offer is no longer valid. I am just wondering about the invariants here:
* if an offer could go away concurrently here (probably not) we should still emit an error, e.g., to a log
* if the offer will always be valid we should just `CHECK` to be explicit.
Does that make sense?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42086/#review113991
-----------------------------------------------------------
On Jan. 11, 2016, 10:31 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42086/
> -----------------------------------------------------------
>
> (Updated Jan. 11, 2016, 10:31 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, and Joris Van Remoortere.
>
>
> Bugs: MESOS-4301
> https://issues.apache.org/jira/browse/MESOS-4301
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds `validation::offer::inverse::validate` to validate inverse offers along the same lines as `validation::offer::validate`, except `SlaveId` is not validated for inverse offers.
>
> Fixes and refactors `Master::accept` to allow `ACCEPT` calls that contain both offers and inverse offers.
> Also tweaks `Master::accept` to not print a misleading log line "ACCEPT call used invalid offers ..." when the call only includes inverse offers.
>
>
> Diffs
> -----
>
> include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a
> src/common/type_utils.cpp 76f48f6a1f5467db032ded8acd296d03353b4172
> src/master/master.hpp f02d165874fa8023675e545793de699aeecae29b
> src/master/master.cpp 2d9b7f9540574aa3ef9e5af3b2b8922dffeebac8
> src/master/validation.hpp 380b40279faf180a6f401a5e28280b601dbc648c
> src/master/validation.cpp 6a43bce5b7df6a9d939245c4726d060fa19eb305
>
> Diff: https://reviews.apache.org/r/42086/diff/
>
>
> Testing
> -------
>
> make check
>
> # Ran the following and checked for blank output:
> bin/mesos-tests.sh --gtest_filter="*Inverse*Offer*" --verbose 2>&1 /dev/null | grep "ACCEPT call used invalid offers"
>
> # Check new test for flakiness:
> bin/mesos-tests.sh --gtest_filter="*OffersAndInverseOffers" --gtest_repeat=1500 --gtest_break_on_failure
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 42086: Updated and refactored Master::accept for
Offers with InverseOffers.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42086/#review113991
-----------------------------------------------------------
src/master/master.cpp (line 3072)
<https://reviews.apache.org/r/42086/#comment174776>
Since there's a clean separation among offers and inverse offers, it seems we could begin logging unknown offers (probably: at some low prio).
src/master/master.cpp (line 3105)
<https://reviews.apache.org/r/42086/#comment174775>
Since there's a clean separation among offers and inverse offers, it seems we could begin logging unknown offers (probably: at some low prio).
- Benjamin Bannier
On Jan. 11, 2016, 10:31 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42086/
> -----------------------------------------------------------
>
> (Updated Jan. 11, 2016, 10:31 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, and Joris Van Remoortere.
>
>
> Bugs: MESOS-4301
> https://issues.apache.org/jira/browse/MESOS-4301
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds `validation::offer::inverse::validate` to validate inverse offers along the same lines as `validation::offer::validate`, except `SlaveId` is not validated for inverse offers.
>
> Fixes and refactors `Master::accept` to allow `ACCEPT` calls that contain both offers and inverse offers.
> Also tweaks `Master::accept` to not print a misleading log line "ACCEPT call used invalid offers ..." when the call only includes inverse offers.
>
>
> Diffs
> -----
>
> include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a
> src/common/type_utils.cpp 76f48f6a1f5467db032ded8acd296d03353b4172
> src/master/master.hpp f02d165874fa8023675e545793de699aeecae29b
> src/master/master.cpp 2d9b7f9540574aa3ef9e5af3b2b8922dffeebac8
> src/master/validation.hpp 380b40279faf180a6f401a5e28280b601dbc648c
> src/master/validation.cpp 6a43bce5b7df6a9d939245c4726d060fa19eb305
>
> Diff: https://reviews.apache.org/r/42086/diff/
>
>
> Testing
> -------
>
> make check
>
> # Ran the following and checked for blank output:
> bin/mesos-tests.sh --gtest_filter="*Inverse*Offer*" --verbose 2>&1 /dev/null | grep "ACCEPT call used invalid offers"
>
> # Check new test for flakiness:
> bin/mesos-tests.sh --gtest_filter="*OffersAndInverseOffers" --gtest_repeat=1500 --gtest_break_on_failure
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 42086: Updated and refactored Master::accept for
Offers with InverseOffers.
Posted by Qian Zhang <zh...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42086/#review114153
-----------------------------------------------------------
src/master/master.cpp (line 3048)
<https://reviews.apache.org/r/42086/#comment174976>
This comment seems not consistent with the code below. In the code, if there are some errors when validating regular offers, we will still go ahead to handle inverse offers, but the comment here says we will reject the entire set of offers.
src/master/master.cpp (line 3067)
<https://reviews.apache.org/r/42086/#comment174978>
So we handle inverse offers even some of them are invalid?
src/master/master.cpp (line 3093)
<https://reviews.apache.org/r/42086/#comment174975>
Do we really care about ```!inverseOfferIds.empty()``` here? I think only checking ```offerIds.empty()``` should be enough.
src/master/master.cpp (line 3132)
<https://reviews.apache.org/r/42086/#comment174984>
Should be offerError.get().message?
- Qian Zhang
On Jan. 13, 2016, 6:20 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42086/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2016, 6:20 a.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, and Joris Van Remoortere.
>
>
> Bugs: MESOS-4301
> https://issues.apache.org/jira/browse/MESOS-4301
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds `validation::offer::inverse::validate` to validate inverse offers along the same lines as `validation::offer::validate`, except `SlaveId` is not validated for inverse offers.
>
> Fixes and refactors `Master::accept` to allow `ACCEPT` calls that contain both offers and inverse offers.
> Also tweaks `Master::accept` to not print a misleading log line "ACCEPT call used invalid offers ..." when the call only includes inverse offers.
>
>
> Diffs
> -----
>
> include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a
> src/common/type_utils.cpp 76f48f6a1f5467db032ded8acd296d03353b4172
> src/master/master.hpp f02d165874fa8023675e545793de699aeecae29b
> src/master/master.cpp 5268408fc63a28afabc27cba96d3ecb360608a65
> src/master/validation.hpp 380b40279faf180a6f401a5e28280b601dbc648c
> src/master/validation.cpp 6a43bce5b7df6a9d939245c4726d060fa19eb305
>
> Diff: https://reviews.apache.org/r/42086/diff/
>
>
> Testing
> -------
>
> make check
>
> # Ran the following and checked for blank output:
> bin/mesos-tests.sh --gtest_filter="*Inverse*Offer*" --verbose 2>&1 /dev/null | grep "ACCEPT call used invalid offers"
>
> # Check new test for flakiness:
> bin/mesos-tests.sh --gtest_filter="*OffersAndInverseOffers" --gtest_repeat=1500 --gtest_break_on_failure
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 42086: Updated and refactored Master::accept for
Offers with InverseOffers.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42086/#review114133
-----------------------------------------------------------
Patch looks great!
Reviews applied: [42092, 42086]
Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
- Mesos ReviewBot
On Jan. 12, 2016, 10:20 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42086/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2016, 10:20 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, and Joris Van Remoortere.
>
>
> Bugs: MESOS-4301
> https://issues.apache.org/jira/browse/MESOS-4301
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds `validation::offer::inverse::validate` to validate inverse offers along the same lines as `validation::offer::validate`, except `SlaveId` is not validated for inverse offers.
>
> Fixes and refactors `Master::accept` to allow `ACCEPT` calls that contain both offers and inverse offers.
> Also tweaks `Master::accept` to not print a misleading log line "ACCEPT call used invalid offers ..." when the call only includes inverse offers.
>
>
> Diffs
> -----
>
> include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a
> src/common/type_utils.cpp 76f48f6a1f5467db032ded8acd296d03353b4172
> src/master/master.hpp f02d165874fa8023675e545793de699aeecae29b
> src/master/master.cpp 5268408fc63a28afabc27cba96d3ecb360608a65
> src/master/validation.hpp 380b40279faf180a6f401a5e28280b601dbc648c
> src/master/validation.cpp 6a43bce5b7df6a9d939245c4726d060fa19eb305
>
> Diff: https://reviews.apache.org/r/42086/diff/
>
>
> Testing
> -------
>
> make check
>
> # Ran the following and checked for blank output:
> bin/mesos-tests.sh --gtest_filter="*Inverse*Offer*" --verbose 2>&1 /dev/null | grep "ACCEPT call used invalid offers"
>
> # Check new test for flakiness:
> bin/mesos-tests.sh --gtest_filter="*OffersAndInverseOffers" --gtest_repeat=1500 --gtest_break_on_failure
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 42086: Updated and refactored Master::accept for
Offers with InverseOffers.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42086/
-----------------------------------------------------------
(Updated Jan. 12, 2016, 2:20 p.m.)
Review request for mesos, Ben Mahler, Artem Harutyunyan, and Joris Van Remoortere.
Changes
-------
Fixed offer validation for unknown offers. These are considered "regular" offers for the purpose of validation.
Bugs: MESOS-4301
https://issues.apache.org/jira/browse/MESOS-4301
Repository: mesos
Description
-------
Adds `validation::offer::inverse::validate` to validate inverse offers along the same lines as `validation::offer::validate`, except `SlaveId` is not validated for inverse offers.
Fixes and refactors `Master::accept` to allow `ACCEPT` calls that contain both offers and inverse offers.
Also tweaks `Master::accept` to not print a misleading log line "ACCEPT call used invalid offers ..." when the call only includes inverse offers.
Diffs (updated)
-----
include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a
src/common/type_utils.cpp 76f48f6a1f5467db032ded8acd296d03353b4172
src/master/master.hpp f02d165874fa8023675e545793de699aeecae29b
src/master/master.cpp 5268408fc63a28afabc27cba96d3ecb360608a65
src/master/validation.hpp 380b40279faf180a6f401a5e28280b601dbc648c
src/master/validation.cpp 6a43bce5b7df6a9d939245c4726d060fa19eb305
Diff: https://reviews.apache.org/r/42086/diff/
Testing
-------
make check
# Ran the following and checked for blank output:
bin/mesos-tests.sh --gtest_filter="*Inverse*Offer*" --verbose 2>&1 /dev/null | grep "ACCEPT call used invalid offers"
# Check new test for flakiness:
bin/mesos-tests.sh --gtest_filter="*OffersAndInverseOffers" --gtest_repeat=1500 --gtest_break_on_failure
Thanks,
Joseph Wu
Re: Review Request 42086: Updated and refactored Master::accept for
Offers with InverseOffers.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42086/#review113952
-----------------------------------------------------------
Patch looks great!
Reviews applied: [42092, 42086]
Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
- Mesos ReviewBot
On Jan. 11, 2016, 9:31 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42086/
> -----------------------------------------------------------
>
> (Updated Jan. 11, 2016, 9:31 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, and Joris Van Remoortere.
>
>
> Bugs: MESOS-4301
> https://issues.apache.org/jira/browse/MESOS-4301
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds `validation::offer::inverse::validate` to validate inverse offers along the same lines as `validation::offer::validate`, except `SlaveId` is not validated for inverse offers.
>
> Fixes and refactors `Master::accept` to allow `ACCEPT` calls that contain both offers and inverse offers.
> Also tweaks `Master::accept` to not print a misleading log line "ACCEPT call used invalid offers ..." when the call only includes inverse offers.
>
>
> Diffs
> -----
>
> include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a
> src/common/type_utils.cpp 76f48f6a1f5467db032ded8acd296d03353b4172
> src/master/master.hpp f02d165874fa8023675e545793de699aeecae29b
> src/master/master.cpp 2d9b7f9540574aa3ef9e5af3b2b8922dffeebac8
> src/master/validation.hpp 380b40279faf180a6f401a5e28280b601dbc648c
> src/master/validation.cpp 6a43bce5b7df6a9d939245c4726d060fa19eb305
>
> Diff: https://reviews.apache.org/r/42086/diff/
>
>
> Testing
> -------
>
> make check
>
> # Ran the following and checked for blank output:
> bin/mesos-tests.sh --gtest_filter="*Inverse*Offer*" --verbose 2>&1 /dev/null | grep "ACCEPT call used invalid offers"
>
> # Check new test for flakiness:
> bin/mesos-tests.sh --gtest_filter="*OffersAndInverseOffers" --gtest_repeat=1500 --gtest_break_on_failure
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 42086: Updated and refactored Master::accept for
Offers with InverseOffers.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42086/
-----------------------------------------------------------
(Updated Jan. 11, 2016, 1:31 p.m.)
Review request for mesos, Ben Mahler, Artem Harutyunyan, and Joris Van Remoortere.
Changes
-------
Shifted one expression down. Reworded some comments.
Bugs: MESOS-4301
https://issues.apache.org/jira/browse/MESOS-4301
Repository: mesos
Description
-------
Adds `validation::offer::inverse::validate` to validate inverse offers along the same lines as `validation::offer::validate`, except `SlaveId` is not validated for inverse offers.
Fixes and refactors `Master::accept` to allow `ACCEPT` calls that contain both offers and inverse offers.
Also tweaks `Master::accept` to not print a misleading log line "ACCEPT call used invalid offers ..." when the call only includes inverse offers.
Diffs (updated)
-----
include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a
src/common/type_utils.cpp 76f48f6a1f5467db032ded8acd296d03353b4172
src/master/master.hpp f02d165874fa8023675e545793de699aeecae29b
src/master/master.cpp 2d9b7f9540574aa3ef9e5af3b2b8922dffeebac8
src/master/validation.hpp 380b40279faf180a6f401a5e28280b601dbc648c
src/master/validation.cpp 6a43bce5b7df6a9d939245c4726d060fa19eb305
Diff: https://reviews.apache.org/r/42086/diff/
Testing
-------
make check
# Ran the following and checked for blank output:
bin/mesos-tests.sh --gtest_filter="*Inverse*Offer*" --verbose 2>&1 /dev/null | grep "ACCEPT call used invalid offers"
# Check new test for flakiness:
bin/mesos-tests.sh --gtest_filter="*OffersAndInverseOffers" --gtest_repeat=1500 --gtest_break_on_failure
Thanks,
Joseph Wu
Re: Review Request 42086: Updated and refactored Master::accept for
Offers with InverseOffers.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Jan. 9, 2016, 1:02 a.m., Klaus Ma wrote:
> > src/master/master.cpp, line 3078
> > <https://reviews.apache.org/r/42086/diff/2/?file=1189007#file1189007line3078>
> >
> > If both `offerError.isSome()` and `inverseOfferError.isSome()`, should we return `Error()`?
>
> Joseph Wu wrote:
> What do you mean? (This method has a `void` return type.)
>
> Klaus Ma wrote:
> I'm thinking what's the behaviour when both offer & inverseOffer failed at validation. Just re-check the code, it behaviour is recoverResources, right?
That's right. Any failure in validation will result in invalidating the offers (and TASK_LOST for all LAUNCH operations).
I'll click "Drop" since this is already present in the code.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42086/#review113610
-----------------------------------------------------------
On Jan. 11, 2016, 1:31 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42086/
> -----------------------------------------------------------
>
> (Updated Jan. 11, 2016, 1:31 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, and Joris Van Remoortere.
>
>
> Bugs: MESOS-4301
> https://issues.apache.org/jira/browse/MESOS-4301
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds `validation::offer::inverse::validate` to validate inverse offers along the same lines as `validation::offer::validate`, except `SlaveId` is not validated for inverse offers.
>
> Fixes and refactors `Master::accept` to allow `ACCEPT` calls that contain both offers and inverse offers.
> Also tweaks `Master::accept` to not print a misleading log line "ACCEPT call used invalid offers ..." when the call only includes inverse offers.
>
>
> Diffs
> -----
>
> include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a
> src/common/type_utils.cpp 76f48f6a1f5467db032ded8acd296d03353b4172
> src/master/master.hpp f02d165874fa8023675e545793de699aeecae29b
> src/master/master.cpp 2d9b7f9540574aa3ef9e5af3b2b8922dffeebac8
> src/master/validation.hpp 380b40279faf180a6f401a5e28280b601dbc648c
> src/master/validation.cpp 6a43bce5b7df6a9d939245c4726d060fa19eb305
>
> Diff: https://reviews.apache.org/r/42086/diff/
>
>
> Testing
> -------
>
> make check
>
> # Ran the following and checked for blank output:
> bin/mesos-tests.sh --gtest_filter="*Inverse*Offer*" --verbose 2>&1 /dev/null | grep "ACCEPT call used invalid offers"
>
> # Check new test for flakiness:
> bin/mesos-tests.sh --gtest_filter="*OffersAndInverseOffers" --gtest_repeat=1500 --gtest_break_on_failure
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 42086: Updated and refactored Master::accept for
Offers with InverseOffers.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Jan. 9, 2016, 1:02 a.m., Klaus Ma wrote:
> > src/master/master.cpp, line 3078
> > <https://reviews.apache.org/r/42086/diff/2/?file=1189007#file1189007line3078>
> >
> > If both `offerError.isSome()` and `inverseOfferError.isSome()`, should we return `Error()`?
What do you mean? (This method has a `void` return type.)
> On Jan. 9, 2016, 1:02 a.m., Klaus Ma wrote:
> > src/master/master.cpp, line 3122
> > <https://reviews.apache.org/r/42086/diff/2/?file=1189007#file1189007line3122>
> >
> > Why did we check `inverseOfferError.isSome()`? inverseOffer should be handled before.
`Master::accept` will process multiple Offers at once, but if there is any error, we reject the entire set of offers. That's why we check both errors here.
I'll reword a few comments to make this more explicit.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42086/#review113610
-----------------------------------------------------------
On Jan. 8, 2016, 5:58 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42086/
> -----------------------------------------------------------
>
> (Updated Jan. 8, 2016, 5:58 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, and Joris Van Remoortere.
>
>
> Bugs: MESOS-4301
> https://issues.apache.org/jira/browse/MESOS-4301
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds `validation::offer::inverse::validate` to validate inverse offers along the same lines as `validation::offer::validate`, except `SlaveId` is not validated for inverse offers.
>
> Fixes and refactors `Master::accept` to allow `ACCEPT` calls that contain both offers and inverse offers.
> Also tweaks `Master::accept` to not print a misleading log line "ACCEPT call used invalid offers ..." when the call only includes inverse offers.
>
>
> Diffs
> -----
>
> include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a
> src/common/type_utils.cpp 76f48f6a1f5467db032ded8acd296d03353b4172
> src/master/master.hpp f02d165874fa8023675e545793de699aeecae29b
> src/master/master.cpp 2d9b7f9540574aa3ef9e5af3b2b8922dffeebac8
> src/master/validation.hpp 380b40279faf180a6f401a5e28280b601dbc648c
> src/master/validation.cpp 6a43bce5b7df6a9d939245c4726d060fa19eb305
>
> Diff: https://reviews.apache.org/r/42086/diff/
>
>
> Testing
> -------
>
> make check
>
> # Ran the following and checked for blank output:
> bin/mesos-tests.sh --gtest_filter="*Inverse*Offer*" --verbose 2>&1 /dev/null | grep "ACCEPT call used invalid offers"
>
> # Check new test for flakiness:
> bin/mesos-tests.sh --gtest_filter="*OffersAndInverseOffers" --gtest_repeat=1500 --gtest_break_on_failure
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 42086: Updated and refactored Master::accept for
Offers with InverseOffers.
Posted by Klaus Ma <kl...@gmail.com>.
> On Jan. 9, 2016, 5:02 p.m., Klaus Ma wrote:
> > src/master/master.cpp, line 3078
> > <https://reviews.apache.org/r/42086/diff/2/?file=1189007#file1189007line3078>
> >
> > If both `offerError.isSome()` and `inverseOfferError.isSome()`, should we return `Error()`?
>
> Joseph Wu wrote:
> What do you mean? (This method has a `void` return type.)
I'm thinking what's the behaviour when both offer & inverseOffer failed at validation. Just re-check the code, it behaviour is recoverResources, right?
> On Jan. 9, 2016, 5:02 p.m., Klaus Ma wrote:
> > src/master/master.cpp, line 3122
> > <https://reviews.apache.org/r/42086/diff/2/?file=1189007#file1189007line3122>
> >
> > Why did we check `inverseOfferError.isSome()`? inverseOffer should be handled before.
>
> Joseph Wu wrote:
> `Master::accept` will process multiple Offers at once, but if there is any error, we reject the entire set of offers. That's why we check both errors here.
>
> I'll reword a few comments to make this more explicit.
For this one, I'd suggest to hanlde inverseOffer's case in its loop; current code logic is handling inverseOffer, then handling offers and if there's inverseOffer, also handle it (recoverResource).
- Klaus
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42086/#review113610
-----------------------------------------------------------
On Jan. 12, 2016, 5:31 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42086/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2016, 5:31 a.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, and Joris Van Remoortere.
>
>
> Bugs: MESOS-4301
> https://issues.apache.org/jira/browse/MESOS-4301
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds `validation::offer::inverse::validate` to validate inverse offers along the same lines as `validation::offer::validate`, except `SlaveId` is not validated for inverse offers.
>
> Fixes and refactors `Master::accept` to allow `ACCEPT` calls that contain both offers and inverse offers.
> Also tweaks `Master::accept` to not print a misleading log line "ACCEPT call used invalid offers ..." when the call only includes inverse offers.
>
>
> Diffs
> -----
>
> include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a
> src/common/type_utils.cpp 76f48f6a1f5467db032ded8acd296d03353b4172
> src/master/master.hpp f02d165874fa8023675e545793de699aeecae29b
> src/master/master.cpp 2d9b7f9540574aa3ef9e5af3b2b8922dffeebac8
> src/master/validation.hpp 380b40279faf180a6f401a5e28280b601dbc648c
> src/master/validation.cpp 6a43bce5b7df6a9d939245c4726d060fa19eb305
>
> Diff: https://reviews.apache.org/r/42086/diff/
>
>
> Testing
> -------
>
> make check
>
> # Ran the following and checked for blank output:
> bin/mesos-tests.sh --gtest_filter="*Inverse*Offer*" --verbose 2>&1 /dev/null | grep "ACCEPT call used invalid offers"
>
> # Check new test for flakiness:
> bin/mesos-tests.sh --gtest_filter="*OffersAndInverseOffers" --gtest_repeat=1500 --gtest_break_on_failure
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 42086: Updated and refactored Master::accept for
Offers with InverseOffers.
Posted by Klaus Ma <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42086/#review113610
-----------------------------------------------------------
src/master/master.cpp (line 3066)
<https://reviews.apache.org/r/42086/#comment174370>
If both `offerError.isSome()` and `inverseOfferError.isSome()`, should we return `Error()`?
src/master/master.cpp (line 3109)
<https://reviews.apache.org/r/42086/#comment174371>
Why did we check `inverseOfferError.isSome()`? inverseOffer should be handled before.
- Klaus Ma
On Jan. 9, 2016, 9:58 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42086/
> -----------------------------------------------------------
>
> (Updated Jan. 9, 2016, 9:58 a.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, and Joris Van Remoortere.
>
>
> Bugs: MESOS-4301
> https://issues.apache.org/jira/browse/MESOS-4301
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds `validation::offer::inverse::validate` to validate inverse offers along the same lines as `validation::offer::validate`, except `SlaveId` is not validated for inverse offers.
>
> Fixes and refactors `Master::accept` to allow `ACCEPT` calls that contain both offers and inverse offers.
> Also tweaks `Master::accept` to not print a misleading log line "ACCEPT call used invalid offers ..." when the call only includes inverse offers.
>
>
> Diffs
> -----
>
> include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a
> src/common/type_utils.cpp 76f48f6a1f5467db032ded8acd296d03353b4172
> src/master/master.hpp f02d165874fa8023675e545793de699aeecae29b
> src/master/master.cpp 2d9b7f9540574aa3ef9e5af3b2b8922dffeebac8
> src/master/validation.hpp 380b40279faf180a6f401a5e28280b601dbc648c
> src/master/validation.cpp 6a43bce5b7df6a9d939245c4726d060fa19eb305
>
> Diff: https://reviews.apache.org/r/42086/diff/
>
>
> Testing
> -------
>
> make check
>
> # Ran the following and checked for blank output:
> bin/mesos-tests.sh --gtest_filter="*Inverse*Offer*" --verbose 2>&1 /dev/null | grep "ACCEPT call used invalid offers"
>
> # Check new test for flakiness:
> bin/mesos-tests.sh --gtest_filter="*OffersAndInverseOffers" --gtest_repeat=1500 --gtest_break_on_failure
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 42086: Updated and refactored Master::accept for
Offers with InverseOffers.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42086/#review113672
-----------------------------------------------------------
Patch looks great!
Reviews applied: [42092, 42086]
Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
- Mesos ReviewBot
On Jan. 9, 2016, 1:58 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42086/
> -----------------------------------------------------------
>
> (Updated Jan. 9, 2016, 1:58 a.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, and Joris Van Remoortere.
>
>
> Bugs: MESOS-4301
> https://issues.apache.org/jira/browse/MESOS-4301
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds `validation::offer::inverse::validate` to validate inverse offers along the same lines as `validation::offer::validate`, except `SlaveId` is not validated for inverse offers.
>
> Fixes and refactors `Master::accept` to allow `ACCEPT` calls that contain both offers and inverse offers.
> Also tweaks `Master::accept` to not print a misleading log line "ACCEPT call used invalid offers ..." when the call only includes inverse offers.
>
>
> Diffs
> -----
>
> include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a
> src/common/type_utils.cpp 76f48f6a1f5467db032ded8acd296d03353b4172
> src/master/master.hpp f02d165874fa8023675e545793de699aeecae29b
> src/master/master.cpp 2d9b7f9540574aa3ef9e5af3b2b8922dffeebac8
> src/master/validation.hpp 380b40279faf180a6f401a5e28280b601dbc648c
> src/master/validation.cpp 6a43bce5b7df6a9d939245c4726d060fa19eb305
>
> Diff: https://reviews.apache.org/r/42086/diff/
>
>
> Testing
> -------
>
> make check
>
> # Ran the following and checked for blank output:
> bin/mesos-tests.sh --gtest_filter="*Inverse*Offer*" --verbose 2>&1 /dev/null | grep "ACCEPT call used invalid offers"
>
> # Check new test for flakiness:
> bin/mesos-tests.sh --gtest_filter="*OffersAndInverseOffers" --gtest_repeat=1500 --gtest_break_on_failure
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 42086: Updated and refactored Master::accept for
Offers with InverseOffers.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Jan. 8, 2016, 7:13 p.m., Guangya Liu wrote:
> > src/master/validation.cpp, lines 678-709
> > <https://reviews.apache.org/r/42086/diff/2/?file=1189009#file1189009line678>
> >
> > I saw that there is no test cases for offer validation, do you want to add inverseOffer test here?
I think the reason we don't have unit tests for `offer::validate` is that the method is not very unit-test-friendly. It takes a `Master*` as an argument and needs to access a chunk (especially `Master::Slave`) that would take quite a bit of code to instantiate in a unit test.
We do have tests that hit this validation code though:
MasterTest.LaunchCombinedOfferTest
MasterTest.LaunchAcrossSlavesTest
MasterTest.LaunchDuplicateOfferTest
Since the inverse offer validation uses the same code, we don't need to duplicate all the tests. I think the regression test (https://reviews.apache.org/r/42092/) should be sufficient.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42086/#review113597
-----------------------------------------------------------
On Jan. 8, 2016, 5:58 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42086/
> -----------------------------------------------------------
>
> (Updated Jan. 8, 2016, 5:58 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, and Joris Van Remoortere.
>
>
> Bugs: MESOS-4301
> https://issues.apache.org/jira/browse/MESOS-4301
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds `validation::offer::inverse::validate` to validate inverse offers along the same lines as `validation::offer::validate`, except `SlaveId` is not validated for inverse offers.
>
> Fixes and refactors `Master::accept` to allow `ACCEPT` calls that contain both offers and inverse offers.
> Also tweaks `Master::accept` to not print a misleading log line "ACCEPT call used invalid offers ..." when the call only includes inverse offers.
>
>
> Diffs
> -----
>
> include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a
> src/common/type_utils.cpp 76f48f6a1f5467db032ded8acd296d03353b4172
> src/master/master.hpp f02d165874fa8023675e545793de699aeecae29b
> src/master/master.cpp 2d9b7f9540574aa3ef9e5af3b2b8922dffeebac8
> src/master/validation.hpp 380b40279faf180a6f401a5e28280b601dbc648c
> src/master/validation.cpp 6a43bce5b7df6a9d939245c4726d060fa19eb305
>
> Diff: https://reviews.apache.org/r/42086/diff/
>
>
> Testing
> -------
>
> make check
>
> # Ran the following and checked for blank output:
> bin/mesos-tests.sh --gtest_filter="*Inverse*Offer*" --verbose 2>&1 /dev/null | grep "ACCEPT call used invalid offers"
>
> # Check new test for flakiness:
> bin/mesos-tests.sh --gtest_filter="*OffersAndInverseOffers" --gtest_repeat=1500 --gtest_break_on_failure
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 42086: Updated and refactored Master::accept for
Offers with InverseOffers.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42086/#review113597
-----------------------------------------------------------
src/master/master.cpp (lines 3054 - 3059)
<https://reviews.apache.org/r/42086/#comment174351>
Move this under L3050 to aggregate all offer validation operations.
src/master/validation.cpp (lines 678 - 709)
<https://reviews.apache.org/r/42086/#comment174352>
I saw that there is no test cases for offer validation, do you want to add inverseOffer test here?
- Guangya Liu
On 一月 9, 2016, 1:58 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42086/
> -----------------------------------------------------------
>
> (Updated 一月 9, 2016, 1:58 a.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, and Joris Van Remoortere.
>
>
> Bugs: MESOS-4301
> https://issues.apache.org/jira/browse/MESOS-4301
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds `validation::offer::inverse::validate` to validate inverse offers along the same lines as `validation::offer::validate`, except `SlaveId` is not validated for inverse offers.
>
> Fixes and refactors `Master::accept` to allow `ACCEPT` calls that contain both offers and inverse offers.
> Also tweaks `Master::accept` to not print a misleading log line "ACCEPT call used invalid offers ..." when the call only includes inverse offers.
>
>
> Diffs
> -----
>
> include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a
> src/common/type_utils.cpp 76f48f6a1f5467db032ded8acd296d03353b4172
> src/master/master.hpp f02d165874fa8023675e545793de699aeecae29b
> src/master/master.cpp 2d9b7f9540574aa3ef9e5af3b2b8922dffeebac8
> src/master/validation.hpp 380b40279faf180a6f401a5e28280b601dbc648c
> src/master/validation.cpp 6a43bce5b7df6a9d939245c4726d060fa19eb305
>
> Diff: https://reviews.apache.org/r/42086/diff/
>
>
> Testing
> -------
>
> make check
>
> # Ran the following and checked for blank output:
> bin/mesos-tests.sh --gtest_filter="*Inverse*Offer*" --verbose 2>&1 /dev/null | grep "ACCEPT call used invalid offers"
>
> # Check new test for flakiness:
> bin/mesos-tests.sh --gtest_filter="*OffersAndInverseOffers" --gtest_repeat=1500 --gtest_break_on_failure
>
>
> Thanks,
>
> Joseph Wu
>
>