You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrei Sekretenko <as...@apache.org> on 2020/09/22 18:05:39 UTC
Review Request 72897: Allowed `OfferConstraintsFilter::create()` to
return an empty `Result`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72897/
-----------------------------------------------------------
Review request for mesos and Benjamin Mahler.
Bugs: MESOS-10189
https://issues.apache.org/jira/browse/MESOS-10189
Repository: mesos
Description
-------
This patch makes it possible for the offer constraint filter creation
to return `None()` in cases when specified offer constraints would
result in creating a no-op `OfferConstraintsFilter` that would never
filter out anything, and actually implements this special handling
for an `OfferConstraints` message with an empty role constraints map.
Diffs
-----
include/mesos/allocator/allocator.hpp 6d67d5d5c32c1f18cdf6f925d747156c2bbc7820
src/master/allocator/mesos/offer_constraints_filter.cpp 441ebc10219bf3bd623fac2bb08945ea9deb7ea3
src/master/master.cpp fefa72d338384b6ccb1bcbbed7192713411035db
src/tests/master/offer_constraints_filter_tests.cpp f80d56c89d937b6a1b261202adefb37a1519bf0d
Diff: https://reviews.apache.org/r/72897/diff/1/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 72897: Made offer constraints filter and protobuf
non-optional inside the code.
Posted by Andrei Sekretenko <as...@apache.org>.
> On Sept. 24, 2020, 7:58 p.m., Benjamin Mahler wrote:
> > include/mesos/allocator/allocator.hpp
> > Line 113 (original), 113-118 (patched)
> > <https://reviews.apache.org/r/72897/diff/2/?file=2239966#file2239966line113>
> >
> > Hm.. is this used? I would think we're always passing in the object to create, even if not set?
There are at least two cases where we would otherwise need to explicitly perform something like
`CHECK_NOTERROR(OfferConstraintsFilter::create({Bytes(0), 0}, {})`:
- recovering a non-subscribed framework from agent's FrameworkInfo
- various locations in tests, especially the hierarchical allocator tests
Given the sheer number of call sites of `Allocator::addFramework` in the testing codebase, we'll have to add some kind of wrapper for that.
I don't think this is better than just keeping framewok's allocator options default-constructable.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72897/#review221952
-----------------------------------------------------------
On Sept. 24, 2020, 5:07 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72897/
> -----------------------------------------------------------
>
> (Updated Sept. 24, 2020, 5:07 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-10189
> https://issues.apache.org/jira/browse/MESOS-10189
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Given that Mesos now provides a guarantee that specifying no offer
> constraint in UPDATE_FRAMEWORK/SUBSCRIBE call is equivalent to
> specifying default-constructed `OfferConstraints`, and that we are
> intending to make the V0 scheduler driver always require offer
> constraints as an argument to the `updateFramework()`, it no longer
> makes sense to keep `OfferConstraints`/`OfferConstraintsFilter`
> optional inside the Mesos code base.
>
> This patch replaces a non-set `Option<OfferConstraints>` with
> default-constructed `OfferConstraints`, and a non-set
> `Option<OfferConstraintsFilter>` with a default-constructed filter.
>
>
> Diffs
> -----
>
> include/mesos/allocator/allocator.hpp b0a5d6aaa08a05cb1ece318999a8760df932ba64
> src/master/allocator/mesos/offer_constraints_filter.cpp 441ebc10219bf3bd623fac2bb08945ea9deb7ea3
> src/master/framework.cpp 980828e04cae96f091a6b30e553ff667fa58a917
> src/master/http.cpp 3d8e7095ee58c2505042095b2c593c2317f3232f
> src/master/master.hpp 95aca586f17a206b6ac7f0c48ea99c8525c336ad
> src/master/master.cpp 576ae107a6a3c99b51516fdde4df9bff86c90a99
> src/master/readonly_handler.cpp df499d1a5e9622222ee2fe0c47bc77fb1ef9d8bc
>
>
> Diff: https://reviews.apache.org/r/72897/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 72897: Made offer constraints filter and protobuf
non-optional inside the code.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72897/#review221952
-----------------------------------------------------------
Ship it!
include/mesos/allocator/allocator.hpp
Line 113 (original), 113-118 (patched)
<https://reviews.apache.org/r/72897/#comment311019>
Hm.. is this used? I would think we're always passing in the object to create, even if not set?
- Benjamin Mahler
On Sept. 24, 2020, 3:07 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72897/
> -----------------------------------------------------------
>
> (Updated Sept. 24, 2020, 3:07 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-10189
> https://issues.apache.org/jira/browse/MESOS-10189
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Given that Mesos now provides a guarantee that specifying no offer
> constraint in UPDATE_FRAMEWORK/SUBSCRIBE call is equivalent to
> specifying default-constructed `OfferConstraints`, and that we are
> intending to make the V0 scheduler driver always require offer
> constraints as an argument to the `updateFramework()`, it no longer
> makes sense to keep `OfferConstraints`/`OfferConstraintsFilter`
> optional inside the Mesos code base.
>
> This patch replaces a non-set `Option<OfferConstraints>` with
> default-constructed `OfferConstraints`, and a non-set
> `Option<OfferConstraintsFilter>` with a default-constructed filter.
>
>
> Diffs
> -----
>
> include/mesos/allocator/allocator.hpp b0a5d6aaa08a05cb1ece318999a8760df932ba64
> src/master/allocator/mesos/offer_constraints_filter.cpp 441ebc10219bf3bd623fac2bb08945ea9deb7ea3
> src/master/framework.cpp 980828e04cae96f091a6b30e553ff667fa58a917
> src/master/http.cpp 3d8e7095ee58c2505042095b2c593c2317f3232f
> src/master/master.hpp 95aca586f17a206b6ac7f0c48ea99c8525c336ad
> src/master/master.cpp 576ae107a6a3c99b51516fdde4df9bff86c90a99
> src/master/readonly_handler.cpp df499d1a5e9622222ee2fe0c47bc77fb1ef9d8bc
>
>
> Diff: https://reviews.apache.org/r/72897/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 72897: Made offer constraints filter and protobuf
non-optional inside the code.
Posted by Andrei Sekretenko <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72897/
-----------------------------------------------------------
(Updated Sept. 24, 2020, 5:07 p.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Removed filter/constraints optionality instead of the previous approach.
Summary (updated)
-----------------
Made offer constraints filter and protobuf non-optional inside the code.
Bugs: MESOS-10189
https://issues.apache.org/jira/browse/MESOS-10189
Repository: mesos
Description (updated)
-------
Given that Mesos now provides a guarantee that specifying no offer
constraint in UPDATE_FRAMEWORK/SUBSCRIBE call is equivalent to
specifying default-constructed `OfferConstraints`, and that we are
intending to make the V0 scheduler driver always require offer
constraints as an argument to the `updateFramework()`, it no longer
makes sense to keep `OfferConstraints`/`OfferConstraintsFilter`
optional inside the Mesos code base.
This patch replaces a non-set `Option<OfferConstraints>` with
default-constructed `OfferConstraints`, and a non-set
`Option<OfferConstraintsFilter>` with a default-constructed filter.
Diffs (updated)
-----
include/mesos/allocator/allocator.hpp b0a5d6aaa08a05cb1ece318999a8760df932ba64
src/master/allocator/mesos/offer_constraints_filter.cpp 441ebc10219bf3bd623fac2bb08945ea9deb7ea3
src/master/framework.cpp 980828e04cae96f091a6b30e553ff667fa58a917
src/master/http.cpp 3d8e7095ee58c2505042095b2c593c2317f3232f
src/master/master.hpp 95aca586f17a206b6ac7f0c48ea99c8525c336ad
src/master/master.cpp 576ae107a6a3c99b51516fdde4df9bff86c90a99
src/master/readonly_handler.cpp df499d1a5e9622222ee2fe0c47bc77fb1ef9d8bc
Diff: https://reviews.apache.org/r/72897/diff/2/
Changes: https://reviews.apache.org/r/72897/diff/1-2/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 72897: Allowed `OfferConstraintsFilter::create()`
to return an empty `Result`.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Sept. 22, 2020, 8:50 p.m., Benjamin Mahler wrote:
> > Hm.. can you clarify what this gives us?
>
> Andrei Sekretenko wrote:
> Now I think I need your input here.
> If we decide to keep in a long term a guarantee that specifying default-constructed OfferConstraints is equivalent to not specifying any constraints at all, we are going to have a certain weirdness internally: right now, there are two functionally equivalent ways to specify to the allocator that the framework has **no offer constraints filtering**.
>
> One way is to set the `offerConstraintsFilter` in the framework's allocator options to `None`; another is to pass a filter constructed from empty constraints.
> My intention was to eliminate one of these two ways. This patch gets rid of the second one (filters that are known to be a no-op are not created).
>
> However, on the second thought, just making a filter non-optional might make things simpler.
>
> I've benchmarked the default (no constraints) case and failed to find any difference between allocator's `struct Framework` always having a non-optional empty filter vs an empty Option.
> No difference - despite of the fact that the first thing in the inner allocation loop is the agent filtering check `offerConstraintsFilter.isSome() && offerConstraintsFilter->isAgentExcluded(..)` (the version that makes filter non-optional drops `isSome()`).
>
> What do you think about just making the filter obligatory (and default-constructable, equivalent to constructing form default `OfferConstraints`)?
>
> Andrei Sekretenko wrote:
> sorry, typo: s/constructing form/constructing from/
Removing the optionality sounds good, and was something I wondered back when seeing the `offerConstraintsFilter.isSome() && offerConstraintsFilter->isAgentExcluded(..)` logic.
The only value I saw from optionality is us knowing (via endpoints) whether the framework is setting the field (and if not set, inferring that the framework is not constraint-capable). i.e. being able to distinguish between a framework that is not constraint capable vs a framework that is constraint capable but doesn't have any constraints to set and therefore uses an empty OfferConstraints.
Thinking about it more though, I don't know that we would see a constraint-capable framework setting it to an empty value very often? Would we?
I'm also not sure whether constraint-capable frameworks will set the field to empty vs not set it. If that's not done consistently then we wouldn't get much value from distinguishnig those cases in the endpoints.
So, I think just removing optionality seems simplest!
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72897/#review221926
-----------------------------------------------------------
On Sept. 22, 2020, 6:05 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72897/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2020, 6:05 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-10189
> https://issues.apache.org/jira/browse/MESOS-10189
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch makes it possible for the offer constraint filter creation
> to return `None()` in cases when specified offer constraints would
> result in creating a no-op `OfferConstraintsFilter` that would never
> filter out anything, and actually implements this special handling
> for an `OfferConstraints` message with an empty role constraints map.
>
>
> Diffs
> -----
>
> include/mesos/allocator/allocator.hpp 6d67d5d5c32c1f18cdf6f925d747156c2bbc7820
> src/master/allocator/mesos/offer_constraints_filter.cpp 441ebc10219bf3bd623fac2bb08945ea9deb7ea3
> src/master/master.cpp fefa72d338384b6ccb1bcbbed7192713411035db
> src/tests/master/offer_constraints_filter_tests.cpp f80d56c89d937b6a1b261202adefb37a1519bf0d
>
>
> Diff: https://reviews.apache.org/r/72897/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 72897: Allowed `OfferConstraintsFilter::create()`
to return an empty `Result`.
Posted by Andrei Sekretenko <as...@apache.org>.
> On Sept. 22, 2020, 10:50 p.m., Benjamin Mahler wrote:
> > Hm.. can you clarify what this gives us?
Now I think I need your input here.
If we decide to keep in a long term a guarantee that specifying default-constructed OfferConstraints is equivalent to not specifying any constraints at all, we are going to have a certain weirdness internally: right now, there are two functionally equivalent ways to specify to the allocator that the framework has **no offer constraints filtering**.
One way is to set the `offerConstraintsFilter` in the framework's allocator options to `None`; another is to pass a filter constructed from empty constraints.
My intention was to eliminate one of these two ways. This patch gets rid of the second one (filters that are known to be a no-op are not created).
However, on the second thought, just making a filter non-optional might make things simpler.
I've benchmarked the default (no constraints) case and failed to find any difference between allocator's `struct Framework` always having a non-optional empty filter vs an empty Option.
No difference - despite of the fact that the first thing in the inner allocation loop is the agent filtering check `offerConstraintsFilter.isSome() && offerConstraintsFilter->isAgentExcluded(..)` (the version that makes filter non-optional drops `isSome()`).
What do you think about just making the filter obligatory (and default-constructable, equivalent to constructing form default `OfferConstraints`)?
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72897/#review221926
-----------------------------------------------------------
On Sept. 22, 2020, 8:05 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72897/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2020, 8:05 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-10189
> https://issues.apache.org/jira/browse/MESOS-10189
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch makes it possible for the offer constraint filter creation
> to return `None()` in cases when specified offer constraints would
> result in creating a no-op `OfferConstraintsFilter` that would never
> filter out anything, and actually implements this special handling
> for an `OfferConstraints` message with an empty role constraints map.
>
>
> Diffs
> -----
>
> include/mesos/allocator/allocator.hpp 6d67d5d5c32c1f18cdf6f925d747156c2bbc7820
> src/master/allocator/mesos/offer_constraints_filter.cpp 441ebc10219bf3bd623fac2bb08945ea9deb7ea3
> src/master/master.cpp fefa72d338384b6ccb1bcbbed7192713411035db
> src/tests/master/offer_constraints_filter_tests.cpp f80d56c89d937b6a1b261202adefb37a1519bf0d
>
>
> Diff: https://reviews.apache.org/r/72897/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 72897: Allowed `OfferConstraintsFilter::create()`
to return an empty `Result`.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72897/#review221926
-----------------------------------------------------------
Hm.. can you clarify what this gives us?
- Benjamin Mahler
On Sept. 22, 2020, 6:05 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72897/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2020, 6:05 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-10189
> https://issues.apache.org/jira/browse/MESOS-10189
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch makes it possible for the offer constraint filter creation
> to return `None()` in cases when specified offer constraints would
> result in creating a no-op `OfferConstraintsFilter` that would never
> filter out anything, and actually implements this special handling
> for an `OfferConstraints` message with an empty role constraints map.
>
>
> Diffs
> -----
>
> include/mesos/allocator/allocator.hpp 6d67d5d5c32c1f18cdf6f925d747156c2bbc7820
> src/master/allocator/mesos/offer_constraints_filter.cpp 441ebc10219bf3bd623fac2bb08945ea9deb7ea3
> src/master/master.cpp fefa72d338384b6ccb1bcbbed7192713411035db
> src/tests/master/offer_constraints_filter_tests.cpp f80d56c89d937b6a1b261202adefb37a1519bf0d
>
>
> Diff: https://reviews.apache.org/r/72897/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>