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 Tue, 18 Aug 2020 18:23:42 GMT


> 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?
> 
> 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)
> > <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.
> 
> Andrei Sekretenko wrote:
>     Documented in https://reviews.apache.org/r/72738/; a test is pending, as I cannot
really test this with Exists constraint.
> 
> Benjamin Mahler wrote:
>     That all sounds reasonable to me! We might want a little TODO there to show the `lookup_mode`
thinking if it becomes an issue.

Added TODO into the protos; the test is in https://reviews.apache.org/r/72776


- Andrei


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


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


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