mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrei Sekretenko <asekrete...@mesosphere.io>
Subject Re: Review Request 72741: Implemented offer constraints filter with Exists/NotExists predicates.
Date Fri, 14 Aug 2020 16:52:46 GMT


> On Aug. 13, 2020, 9:22 p.m., Benjamin Mahler wrote:
> > src/master/offer_constraints_filter.cpp
> > Lines 64 (patched)
> > <https://reviews.apache.org/r/72741/diff/1/?file=2237295#file2237295line64>
> >
> >     This is a little confusing to me since we already have a predicate type from
the protobuf and this is essentially the same thing but in a more usable format for this code?
> >     
> >     I wonder if there's a way to accomplish what we want without the extra predicate
type. Since the evaluator holds one of these predicates, maybe the evaluator can be the one
to hold any extra predicate logic / information needed rather than having a duplicate predicate
type.

I would rather keep constraint evaluation decomposed into attribute selection and predicate
evaluation; the reason is that equality/regexp match will add noticeable amount of code into
the predicate evaluation.


> On Aug. 13, 2020, 9:22 p.m., Benjamin Mahler wrote:
> > src/master/offer_constraints_filter.cpp
> > Lines 134-143 (patched)
> > <https://reviews.apache.org/r/72741/diff/1/?file=2237295#file2237295line134>
> >
> >     I think we allow multiple entries for the same attribute key, on the agent side?
Not sure if anyone uses that.
> 
> Andrei Sekretenko wrote:
>     Good point. 
>     We allow, and this is valid. I don't see any code/document disallowing that; moreover,
IIRC some doc mentions this explicitly.
>     
>     Unconditionally taking the first attribute with the specified name is what Marathon
uses for implementing their placement constraints. 
>     Given that Marathon seems to be the first candidate to make use of constraints-based
filtering, I'm following this approach.
>     
>     Looks like I should document this explicitly and write a test for applying constraint
to an agent with multiple attributes having the same name.
>     If someone at some point will need another logic for the attribute lookup, they will
probably be able to do that by adding something like `lookup_mode` to the `Selector` message
and implementing it here.

Documented in https://reviews.apache.org/r/72738/; a test is pending, as I cannot really test
this with Exists constraint.


- Andrei


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72741/#review221571
-----------------------------------------------------------


On Aug. 14, 2020, 4:42 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72741/
> -----------------------------------------------------------
> 
> (Updated Aug. 14, 2020, 4:42 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-10171
>     https://issues.apache.org/jira/browse/MESOS-10171
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch implements an offer filtering object that supports the
> Exists/NotExists offer constraints, and adds it into the allocator
> interface.
> 
> More constraints will be added to this filter in further patches.
> 
> 
> Diffs
> -----
> 
>   include/mesos/allocator/allocator.hpp c700528e14bb42f6cea37f42dd7b72eb76a1a6b9 
>   src/CMakeLists.txt 4e15e3d99aa2cce2403fe07e762fef2fb4a27dea 
>   src/Makefile.am 447db323875e4cad46000977f4a61600baff8f89 
>   src/master/allocator/mesos/offer_constraints_filter.cpp PRE-CREATION 
>   src/master/master.cpp 6a013e334b19bd108791d1c5fd0864c710aae8cb 
> 
> 
> Diff: https://reviews.apache.org/r/72741/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message