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