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