> On Aug. 13, 2020, 9:22 p.m., Benjamin Mahler wrote:
> > src/master/offer_constraints_filter.cpp
> > Lines 43-56 (patched)
> > <https://reviews.apache.org/r/72741/diff/1/?file=2237295#file2237295line43>
> >
> >     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)
> > <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.

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