mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 72741: Implemented offer constraints filter with Exists/NotExists predicates.
Date Mon, 17 Aug 2020 22:03:04 GMT

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


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