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 17:42:02 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...

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


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