> 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.
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)
> > <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.
That all sounds reasonable to me! We might want a little TODO there to show the `lookup_mode`
thinking if it becomes an issue.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72741/#review221571
-----------------------------------------------------------
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 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/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
|