> On Aug. 17, 2020, 10:03 p.m., Benjamin Mahler wrote:
> > include/mesos/allocator/allocator.hpp
> > Lines 76 (patched)
> > <https://reviews.apache.org/r/72741/diff/4/?file=2238086#file2238086line76>
> >
> >     Hm.. not obvious to me why we need this, can you explain in a comment?

Changed to the alternative (`=default`ed constructors/destructor after `OfferConstraintsFilterImpl`), hopefully that one requres fewer comments.

Please take a look; maybe even that alternative should be explained more extensively than I do in this patch.


> On Aug. 17, 2020, 10:03 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/offer_constraints_filter.cpp
> > Lines 83-93 (patched)
> > <https://reviews.apache.org/r/72741/diff/4/?file=2238089#file2238089line83>
> >
> >     Ditto here for using the enum version, then there's no need for the NOTE at all :)
> >     
> >     ```
> >       static Try<AttributeConstraintPredicate> create(
> >           AttributeConstraint::Predicate&& predicate)
> >       {
> >         using Self = AttributeConstraintPredicate;
> >     
> >         switch (predicate.predicate_case()) {
> >           case kExists:            return Self(Exists{});
> >           case kNotExists:         return Self(NotExists{});
> >           case ONEOF_NAME_NOT_SET: return Error("Unknown predicate type");
> >         }
> >       }
> >     ```
> >     
> >     (Not sure if compiler will see that there's always a return, so I guess the last one might need to break and return Error outside.

I'm not sure all compilers will see that. Added UNREACHABLE().


> On Aug. 17, 2020, 10:03 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/offer_constraints_filter.cpp
> > Lines 99-113 (patched)
> > <https://reviews.apache.org/r/72741/diff/4/?file=2238089#file2238089line99>
> >
> >     I don't know if we're saving much here with the additional abstraction:
> >     
> >     ```
> >       // The following helper structs can apply the predicate using
> >       // overloads for the three cases:
> >       //   (1) Nothing   -> non existent (pseudo) attribute
> >       //   (2) string    -> pseudo attribute value
> >       //   (3) Attribute -> named attribute value
> >     
> >       struct Exists
> >       {
> >         bool apply(const Nothing&) const { return false; }
> >         bool apply(const string&) const { return true; }
> >         bool apply(const Attribute&) const { return true; }
> >       };
> >       
> >       struct NotExists
> >       {
> >         bool apply(const Nothing&) const { return true; }
> >         bool apply(const string&) const { return false; }
> >         bool apply(const Attribute&) const { return false; }
> >       };
> >     ```
> >     
> >     I find thinking about the correctness of `negated` not that trivial vs this direct version. Maybe for others we add later the negated template will be worth it, but for Exists/NotExists it doesn't seem worth it?

Removed this layer. 
Now that the text equality/match constraints are NOT exact negation (the counterparts in pairs behave identically for existing non-TEXT) and thus are not templated, this one doesn't help improve consistency.


- Andrei


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


On Aug. 18, 2020, 6:01 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72741/
> -----------------------------------------------------------
> 
> (Updated Aug. 18, 2020, 6:01 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 a976dc12328f42d2268b4b5d86a934bf0c754594 
>   src/Makefile.am 6d68ed050f99889c142d49bbc72a9292ef64c836 
>   src/master/allocator/mesos/offer_constraints_filter.cpp PRE-CREATION 
>   src/master/master.cpp 6a013e334b19bd108791d1c5fd0864c710aae8cb 
> 
> 
> Diff: https://reviews.apache.org/r/72741/diff/5/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>