----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72741/#review221607 -----------------------------------------------------------
Fix it, then Ship it! include/mesos/allocator/allocator.hpp Lines 76 (patched) <https://reviews.apache.org/r/72741/#comment310669> Hm.. not obvious to me why we need this, can you explain in a comment? include/mesos/allocator/allocator.hpp Lines 94 (patched) <https://reviews.apache.org/r/72741/#comment310670> ) {} include/mesos/allocator/allocator.hpp Lines 96 (patched) <https://reviews.apache.org/r/72741/#comment310671> Can you put the public API section first per our usual convention? src/master/allocator/mesos/offer_constraints_filter.cpp Lines 19 (patched) <https://reviews.apache.org/r/72741/#comment310672> `using` clause for this and s/std::// below? src/master/allocator/mesos/offer_constraints_filter.cpp Lines 45-67 (patched) <https://reviews.apache.org/r/72741/#comment310673> Just re-iterating from the discussion, we could as a general rule access oneofs using the enum approach to make it clear that only 1 can be set at a time: ``` static Option<Error> validate(const Selector& selector) { switch (selector.selector_case()) { case PseudoattributeType: switch (selector.pseudoattribute_type()) { case Selector::HOSTNAME: case Selector::REGION: case Selector::ZONE: return None(); case Selector::UNKNOWN: return Error("Unknown pseudoattribute type"); } case AttributeName: return None(); case ONEOF_NAME_NOT_SET: return Error("Exactly one of 'AttributeConstraint::Selector::name' or" " 'AttributeConstraint::Selector::pseudoattribute_type' must be set"); } } ``` Pretty clean too! src/master/allocator/mesos/offer_constraints_filter.cpp Lines 83-93 (patched) <https://reviews.apache.org/r/72741/#comment310674> Ditto here for using the enum version, then there's no need for the NOTE at all :) ``` static Try<AttributeConstraintPredicate> create( AttributeConstraint::Predicate&& predicate) { using Self = AttributeConstraintPredicate; switch (predicate.predicate_case()) { case kExists: return Self(Exists{}); case kNotExists: return Self(NotExists{}); case ONEOF_NAME_NOT_SET: return Error("Unknown predicate type"); } } ``` (Not sure if compiler will see that there's always a return, so I guess the last one might need to break and return Error outside. src/master/allocator/mesos/offer_constraints_filter.cpp Lines 99-113 (patched) <https://reviews.apache.org/r/72741/#comment310675> I don't know if we're saving much here with the additional abstraction: ``` // The following helper structs can apply the predicate using // overloads for the three cases: // (1) Nothing -> non existent (pseudo) attribute // (2) string -> pseudo attribute value // (3) Attribute -> named attribute value struct Exists { bool apply(const Nothing&) const { return false; } bool apply(const string&) const { return true; } bool apply(const Attribute&) const { return true; } }; struct NotExists { bool apply(const Nothing&) const { return true; } bool apply(const string&) const { return false; } bool apply(const Attribute&) const { return false; } }; ``` I find thinking about the correctness of `negated` not that trivial vs this direct version. Maybe for others we add later the negated template will be worth it, but for Exists/NotExists it doesn't seem worth it? src/master/allocator/mesos/offer_constraints_filter.cpp Lines 124 (patched) <https://reviews.apache.org/r/72741/#comment310676> s/attr/attribute/ ? src/master/allocator/mesos/offer_constraints_filter.cpp Lines 142-174 (patched) <https://reviews.apache.org/r/72741/#comment310677> Ditto here w.r.t. enum usage for oneof - Benjamin Mahler 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 > >
