Re: Review Request 72741: Implemented offer constraints filter with Exists/NotExists predicates.

[Benjamin Mahler]

> On Aug. 17, 2020, 10:03 p.m., Benjamin Mahler wrote:
> > include/mesos/allocator/allocator.hpp
> > Lines 76 (patched)
> > 
> >
> > Hm.. not obvious to me why we need this, can you explain in a comment?
> 
> Andrei Sekretenko wrote:
> 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.

Looks good


- Benjamin


---
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
> 
>

Re: Review Request 72741: Implemented offer constraints filter with Exists/NotExists predicates.

[Andrei Sekretenko]

> On Aug. 13, 2020, 9:22 p.m., Benjamin Mahler wrote:
> > src/master/offer_constraints_filter.cpp
> > Lines 43-56 (patched)
> > 
> >
> > We should check that they aren't both set?
> 
> Andrei Sekretenko wrote:
> `oneof` guarantees that only one is set, regardless of the contents of 
> the serialized message.
> 
> And, at the very least, the code generated by the bundled protoc clearly 
> guarantees that only one of `has_*()` will return `true`. 
> For example, a message like
> ```
> message Foo{} 
>   
> message Bar{} 
>   
>   
>   
> message Msg{  
>   
>   oneof oneofname {   
>   
> Foo foo = 1;  
>   
> Bar bar = 2;  
>   
>   }   
>   
> }
> ```
> results in the following generated code:
> 
> ```  
> class Msg{
>   enum OneofnameCase {
>   
> kFoo = 1, 
>   
> kBar = 2, 
>   
> ONEOFNAME_NOT_SET = 0,
>   
>   };
>   ...
> }
> 
> inline bool Msg::has_foo() const {
>   
>   return oneofname_case() == kFoo;
>   
> } 
> inline Msg::OneofnameCase Msg::oneofname_case() const {   
>   
>   return Msg::OneofnameCase(_oneof_case_[0]); 
>   
> }
> ...
> 
> ```   
> 
> This means that such a check would protect only against two things:
>  - a major and **obvious** bug in `protoc`; if the current implementation 
> has any oneof bugs, this check will not help catch them
>  - us suddenly removing `oneof`; in this case we have bigger problems 
> than this one
> 
> The worst thing about adding this check is that right after adding 
> existence/equality constraiunts it will turn into checking that only one of 
> the **six** fields is set.
> Had we planned to have only two fields forever, this check would have 
> added clarity; with six fields, I doubt it makes things clearer.
> 
> Probably I should add a comment to remind readers that this is `oneof`, 
> or, better, some local static assertion that those fields are part of oneof.
> 
> I have to say that it is rather unfortunate that oneof field 
> accessors/setters have the same names and signatures as those of normal 
> fields...
> 
> Andrei Sekretenko wrote:
> Oops, I was looking at another oneof (for the predicate, which will grow 
> to six members soon). 
> Added a comment for that one, but will add a CHECK here, as we aren't 
> going to have more than two members in the foreseeable future in this one.
> 
> Benjamin Mahler wrote:
> Ah thanks for explaining this! I totally forgot that it was a one of 
> since the code here doesn't reveal it.
> 
> As a general rule for using oneof in protobuf, couldn't we leverage the 
> enum to make it clear in our code that only 1 can be set?
> 
> ```
> switch (selector_case()) {
>   case PseudoattributeType:
> ...
> break;
>   case attributeName:
> ...
> break;
>   case ONEOF_NAME_NOT_SET:
> ...
> break;
> }
> ```

We can, and we should. 
The code around CSI volume-related oneofs already does that; not sure how I 
overlooked that `_case()` is, in fact, well-documented in the protobuf docs.

That said, there is only one `oneof` in Mesos for which this convention is not 
used: `ContainerFileOperation::parameters`.


> On Aug. 13, 2020, 9:22 p.m., Benjamin Mahler wrote:
> > src/master/offer_constraints_filter.cpp
> > Lines 134-143 (patched)
> > 
> >
> > 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 

Re: Review Request 72741: Implemented offer constraints filter with Exists/NotExists predicates.

[Andrei Sekretenko]

> On Aug. 17, 2020, 10:03 p.m., Benjamin Mahler wrote:
> > include/mesos/allocator/allocator.hpp
> > Lines 76 (patched)
> > 
> >
> > 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)
> > 
> >
> > Ditto here for using the enum version, then there's no need for the 
> > NOTE at all :)
> > 
> > ```
> >   static Try 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)
> > 
> >
> > 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
> 
>

Re: Review Request 72741: Implemented offer constraints filter with Exists/NotExists predicates.

[Andrei Sekretenko]

---
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 (updated)
-

  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/

Changes: https://reviews.apache.org/r/72741/diff/4-5/


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 72741: Implemented offer constraints filter with Exists/NotExists predicates.

[Benjamin Mahler]

> On Aug. 13, 2020, 9:22 p.m., Benjamin Mahler wrote:
> > src/master/offer_constraints_filter.cpp
> > Lines 43-56 (patched)
> > 
> >
> > We should check that they aren't both set?
> 
> Andrei Sekretenko wrote:
> `oneof` guarantees that only one is set, regardless of the contents of 
> the serialized message.
> 
> And, at the very least, the code generated by the bundled protoc clearly 
> guarantees that only one of `has_*()` will return `true`. 
> For example, a message like
> ```
> message Foo{} 
>   
> message Bar{} 
>   
>   
>   
> message Msg{  
>   
>   oneof oneofname {   
>   
> Foo foo = 1;  
>   
> Bar bar = 2;  
>   
>   }   
>   
> }
> ```
> results in the following generated code:
> 
> ```  
> class Msg{
>   enum OneofnameCase {
>   
> kFoo = 1, 
>   
> kBar = 2, 
>   
> ONEOFNAME_NOT_SET = 0,
>   
>   };
>   ...
> }
> 
> inline bool Msg::has_foo() const {
>   
>   return oneofname_case() == kFoo;
>   
> } 
> inline Msg::OneofnameCase Msg::oneofname_case() const {   
>   
>   return Msg::OneofnameCase(_oneof_case_[0]); 
>   
> }
> ...
> 
> ```   
> 
> This means that such a check would protect only against two things:
>  - a major and **obvious** bug in `protoc`; if the current implementation 
> has any oneof bugs, this check will not help catch them
>  - us suddenly removing `oneof`; in this case we have bigger problems 
> than this one
> 
> The worst thing about adding this check is that right after adding 
> existence/equality constraiunts it will turn into checking that only one of 
> the **six** fields is set.
> Had we planned to have only two fields forever, this check would have 
> added clarity; with six fields, I doubt it makes things clearer.
> 
> Probably I should add a comment to remind readers that this is `oneof`, 
> or, better, some local static assertion that those fields are part of oneof.
> 
> I have to say that it is rather unfortunate that oneof field 
> accessors/setters have the same names and signatures as those of normal 
> fields...
> 
> Andrei Sekretenko wrote:
> Oops, I was looking at another oneof (for the predicate, which will grow 
> to six members soon). 
> Added a comment for that one, but will add a CHECK here, as we aren't 
> going to have more than two members in the foreseeable future in this one.

Ah thanks for explaining this! I totally forgot that it was a one of since the 
code here doesn't reveal it.

As a general rule for using oneof in protobuf, couldn't we leverage the enum to 
make it clear in our code that only 1 can be set?

```
switch (selector_case()) {
  case PseudoattributeType:
...
break;
  case attributeName:
...
break;
  case ONEOF_NAME_NOT_SET:
...
break;
}
```


> On Aug. 13, 2020, 9:22 p.m., Benjamin Mahler wrote:
> > src/master/offer_constraints_filter.cpp
> > Lines 134-143 (patched)
> > 
> >
> > 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 

Re: Review Request 72741: Implemented offer constraints filter with Exists/NotExists predicates.

[Benjamin Mahler]

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


Fix it, then Ship it!





include/mesos/allocator/allocator.hpp
Lines 76 (patched)


Hm.. not obvious to me why we need this, can you explain in a comment?



include/mesos/allocator/allocator.hpp
Lines 94 (patched)


) {}



include/mesos/allocator/allocator.hpp
Lines 96 (patched)


Can you put the public API section first per our usual convention?



src/master/allocator/mesos/offer_constraints_filter.cpp
Lines 19 (patched)


`using` clause for this and s/std::// below?



src/master/allocator/mesos/offer_constraints_filter.cpp
Lines 45-67 (patched)


Just re-iterating from the discussion, we could as a general rule access 
oneofs using the enum approach to make it clear that only 1 can be set at a 
time:

```
static Option validate(const Selector& selector)
{
  switch (selector.selector_case()) {
case PseudoattributeType:
  switch (selector.pseudoattribute_type()) {
case Selector::HOSTNAME:
case Selector::REGION:
case Selector::ZONE:
  return None();
case Selector::UNKNOWN:
  return Error("Unknown pseudoattribute type");
  }
case AttributeName:
  return None();
case ONEOF_NAME_NOT_SET:
  return Error("Exactly one of 'AttributeConstraint::Selector::name' or"
" 'AttributeConstraint::Selector::pseudoattribute_type' must be 
set");
  }
}
```

Pretty clean too!



src/master/allocator/mesos/offer_constraints_filter.cpp
Lines 83-93 (patched)


Ditto here for using the enum version, then there's no need for the NOTE at 
all :)

```
  static Try 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.



src/master/allocator/mesos/offer_constraints_filter.cpp
Lines 99-113 (patched)


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?



src/master/allocator/mesos/offer_constraints_filter.cpp
Lines 124 (patched)


s/attr/attribute/ ?



src/master/allocator/mesos/offer_constraints_filter.cpp
Lines 142-174 (patched)


Ditto here w.r.t. enum usage for oneof


- Benjamin Mahler


On Aug. 14, 2020, 5:48 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, 5:48 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 

Re: Review Request 72741: Implemented offer constraints filter with Exists/NotExists predicates.

[Andrei Sekretenko]

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

(Updated Aug. 14, 2020, 5:48 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Added a CHECK that the oneof in `Selector` reports at most one field as set.


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 (updated)
-

  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/4/

Changes: https://reviews.apache.org/r/72741/diff/3-4/


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 72741: Implemented offer constraints filter with Exists/NotExists predicates.

[Andrei Sekretenko]

> On Aug. 13, 2020, 9:22 p.m., Benjamin Mahler wrote:
> > src/master/offer_constraints_filter.cpp
> > Lines 43-56 (patched)
> > 
> >
> > We should check that they aren't both set?
> 
> Andrei Sekretenko wrote:
> `oneof` guarantees that only one is set, regardless of the contents of 
> the serialized message.
> 
> And, at the very least, the code generated by the bundled protoc clearly 
> guarantees that only one of `has_*()` will return `true`. 
> For example, a message like
> ```
> message Foo{} 
>   
> message Bar{} 
>   
>   
>   
> message Msg{  
>   
>   oneof oneofname {   
>   
> Foo foo = 1;  
>   
> Bar bar = 2;  
>   
>   }   
>   
> }
> ```
> results in the following generated code:
> 
> ```  
> class Msg{
>   enum OneofnameCase {
>   
> kFoo = 1, 
>   
> kBar = 2, 
>   
> ONEOFNAME_NOT_SET = 0,
>   
>   };
>   ...
> }
> 
> inline bool Msg::has_foo() const {
>   
>   return oneofname_case() == kFoo;
>   
> } 
> inline Msg::OneofnameCase Msg::oneofname_case() const {   
>   
>   return Msg::OneofnameCase(_oneof_case_[0]); 
>   
> }
> ...
> 
> ```   
> 
> This means that such a check would protect only against two things:
>  - a major and **obvious** bug in `protoc`; if the current implementation 
> has any oneof bugs, this check will not help catch them
>  - us suddenly removing `oneof`; in this case we have bigger problems 
> than this one
> 
> The worst thing about adding this check is that right after adding 
> existence/equality constraiunts it will turn into checking that only one of 
> the **six** fields is set.
> Had we planned to have only two fields forever, this check would have 
> added clarity; with six fields, I doubt it makes things clearer.
> 
> Probably I should add a comment to remind readers that this is `oneof`, 
> or, better, some local static assertion that those fields are part of oneof.
> 
> I have to say that it is rather unfortunate that oneof field 
> accessors/setters have the same names and signatures as those of normal 
> fields...

Oops, I was looking at another oneof (for the predicate, which will grow to six 
members soon). 
Added a comment for that one, but will add a CHECK here, as we aren't going to 
have more than two members in the foreseeable future in this one.


- Andrei


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


On Aug. 14, 2020, 5:38 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, 5:38 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/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 72741: Implemented offer constraints filter with Exists/NotExists predicates.

[Andrei Sekretenko]

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

(Updated Aug. 14, 2020, 5:38 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Added a comment explaining how this code relies on `oneof` contract.


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 (updated)
-

  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/3/

Changes: https://reviews.apache.org/r/72741/diff/2-3/


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 72741: Implemented offer constraints filter with Exists/NotExists predicates.

[Andrei Sekretenko]

> On Aug. 13, 2020, 9:22 p.m., Benjamin Mahler wrote:
> > src/master/offer_constraints_filter.cpp
> > Lines 64 (patched)
> > 
> >
> > 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)
> > 
> >
> > 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
> 
>

Re: Review Request 72741: Implemented offer constraints filter with Exists/NotExists predicates.

[Andrei Sekretenko]

---
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.


Changes
---

- Incorporated reworked part of https://reviews.apache.org/r/72740 (I'm 
discarding that PR)
 - Now that the `OfferConstraintsFilter` is declared in 
`include/../allocator.hpp`, I'm relocating `offer_constraints_filter.cpp`
 - addressed feedback on this one


Bugs: MESOS-10171
https://issues.apache.org/jira/browse/MESOS-10171


Repository: mesos


Description (updated)
---

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 (updated)
-

  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/

Changes: https://reviews.apache.org/r/72741/diff/1-2/


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 72741: Implemented offer constraints filter with Exists/NotExists predicates.

[Andrei Sekretenko]

> On Aug. 13, 2020, 9:22 p.m., Benjamin Mahler wrote:
> > src/master/offer_constraints_filter.cpp
> > Lines 43-56 (patched)
> > 
> >
> > We should check that they aren't both set?

`oneof` guarantees that only one is set, regardless of the contents of the 
serialized message.

And, at the very least, the code generated by the bundled protoc clearly 
guarantees that only one of `has_*()` will return `true`. 
For example, a message like
```
message Foo{}   
message Bar{}   

message Msg{
  oneof oneofname { 
Foo foo = 1;
Bar bar = 2;
  } 
}
```
results in the following generated code:

```  
class Msg{
  enum OneofnameCase {  
kFoo = 1,   
kBar = 2,   
ONEOFNAME_NOT_SET = 0,  
  };
  ...
}

inline bool Msg::has_foo() const {  
  return oneofname_case() == kFoo;  
} 
inline Msg::OneofnameCase Msg::oneofname_case() const { 
  return Msg::OneofnameCase(_oneof_case_[0]);   
}
...

``` 
  
This means that such a check would protect only against two things:
 - a major and **obvious** bug in `protoc`; if the current implementation has 
any oneof bugs, this check will not help catch them
 - us suddenly removing `oneof`; in this case we have bigger problems than this 
one

The worst thing about adding this check is that right after adding 
existence/equality constraiunts it will turn into checking that only one of the 
**six** fields is set.
Had we planned to have only two fields forever, this check would have added 
clarity; with six fields, I doubt it makes things clearer.

Probably I should add a comment to remind readers that this is `oneof`, or, 
better, some local static assertion that those fields are part of oneof.

I have to say that it is rather unfortunate that oneof field accessors/setters 
have the same names and signatures as those of normal fields...


> On Aug. 13, 2020, 9:22 p.m., Benjamin Mahler wrote:
> > src/master/offer_constraints_filter.cpp
> > Lines 134-143 (patched)
> > 
> >
> > I think we allow multiple entries for the same attribute key, on the 
> > agent side? Not sure if anyone uses that.

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.


- Andrei


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


On Aug. 6, 2020, 4:24 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72741/
> ---
> 
> (Updated Aug. 6, 2020, 4:24 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 a subclass of AgentResourcesFilter that suppoorts
> the Exists/NotExists offer constraints.
> 
> More constraints will be added to this filter in further patches.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 

Re: Review Request 72741: Implemented offer constraints filter with Exists/NotExists predicates.

[Benjamin Mahler]

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




src/master/offer_constraints_filter.cpp
Lines 43-56 (patched)


We should check that they aren't both set?



src/master/offer_constraints_filter.cpp
Lines 44-51 (patched)


Can we use a switch here per our approach to protobuf enum handling? That 
gives us the benefit of a compiler error when we miss a case (now or later)



src/master/offer_constraints_filter.cpp
Lines 59-60 (patched)


Use single quotes here, and ideally a clear field path:

Exactly one of 'AttributeConstraint::Selector::name' of 
'AttributeConstraint::Selector::pseudoattribute_type' must be set



src/master/offer_constraints_filter.cpp
Lines 64 (patched)


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.



src/master/offer_constraints_filter.cpp
Lines 121 (patched)


braces on the next line (looks like some other classes functions need to be 
updated in this file)



src/master/offer_constraints_filter.cpp
Lines 134-143 (patched)


I think we allow multiple entries for the same attribute key, on the agent 
side? Not sure if anyone uses that.



src/master/offer_constraints_filter.cpp
Lines 147 (patched)


switch (



src/master/offer_constraints_filter.cpp
Lines 181 (patched)


Ditto, don't think we need to bother with the std move here?



src/master/offer_constraints_filter.cpp
Lines 192-195 (patched)


I think we generally put the public interface first, followed by an 
explicit private section?

Ditto in the other case(s) above.



src/master/offer_constraints_filter.cpp
Lines 237 (patched)


hm.. maybe do the role insertion above this loop and have a reference to 
the vector? Seems cleaner and cheaper?

```
foreachpair (
const string& role,
OfferConstraints::RoleConstrints& roleConstraints,
*constraints.mutable_role_constraints()) {
  vector& groups = expressions[role];
  
  for (OfferConstraints::RoleConstraints::Group& group :
   *roleConstraints.mutable_groups()) {
...

groups.emplace_back(std::move(*evaluator));
  }
}
```



src/master/offer_constraints_filter.cpp
Lines 244 (patched)


Don't think we need to worry about the cost benefit of std moving the error 
message to bother with the extra code? It will only be done once, and only in 
the error case.


- Benjamin Mahler


On Aug. 6, 2020, 4:24 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72741/
> ---
> 
> (Updated Aug. 6, 2020, 4:24 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 a subclass of AgentResourcesFilter that suppoorts
> the Exists/NotExists offer constraints.
> 
> More constraints will be added to this filter in further patches.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 4e15e3d99aa2cce2403fe07e762fef2fb4a27dea 
>   src/Makefile.am 447db323875e4cad46000977f4a61600baff8f89 
>   src/master/offer_constraints_filter.hpp PRE-CREATION 
>   src/master/offer_constraints_filter.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/72741/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>