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 Thu, 13 Aug 2020 21:22:59 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72741/#review221571
-----------------------------------------------------------




src/master/offer_constraints_filter.cpp
Lines 43-56 (patched)
<https://reviews.apache.org/r/72741/#comment310637>

    We should check that they aren't both set?



src/master/offer_constraints_filter.cpp
Lines 44-51 (patched)
<https://reviews.apache.org/r/72741/#comment310635>

    Can we use a switch here per our approach to protobuf enum handling? That gives us the
benefit of a compiler error when we miss a case (now or later)



src/master/offer_constraints_filter.cpp
Lines 59-60 (patched)
<https://reviews.apache.org/r/72741/#comment310636>

    Use single quotes here, and ideally a clear field path:
    
    Exactly one of 'AttributeConstraint::Selector::name' of 'AttributeConstraint::Selector::pseudoattribute_type'
must be set



src/master/offer_constraints_filter.cpp
Lines 64 (patched)
<https://reviews.apache.org/r/72741/#comment310638>

    This is a little confusing to me since we already have a predicate type from the protobuf
and this is essentially the same thing but in a more usable format for this code?
    
    I wonder if there's a way to accomplish what we want without the extra predicate type.
Since the evaluator holds one of these predicates, maybe the evaluator can be the one to hold
any extra predicate logic / information needed rather than having a duplicate predicate type.



src/master/offer_constraints_filter.cpp
Lines 121 (patched)
<https://reviews.apache.org/r/72741/#comment310634>

    braces on the next line (looks like some other classes functions need to be updated in
this file)



src/master/offer_constraints_filter.cpp
Lines 134-143 (patched)
<https://reviews.apache.org/r/72741/#comment310639>

    I think we allow multiple entries for the same attribute key, on the agent side? Not sure
if anyone uses that.



src/master/offer_constraints_filter.cpp
Lines 147 (patched)
<https://reviews.apache.org/r/72741/#comment310640>

    switch (



src/master/offer_constraints_filter.cpp
Lines 181 (patched)
<https://reviews.apache.org/r/72741/#comment310643>

    Ditto, don't think we need to bother with the std move here?



src/master/offer_constraints_filter.cpp
Lines 192-195 (patched)
<https://reviews.apache.org/r/72741/#comment310633>

    I think we generally put the public interface first, followed by an explicit private section?
    
    Ditto in the other case(s) above.



src/master/offer_constraints_filter.cpp
Lines 237 (patched)
<https://reviews.apache.org/r/72741/#comment310631>

    hm.. maybe do the role insertion above this loop and have a reference to the vector? Seems
cleaner and cheaper?
    
    ```
    foreachpair (
        const string& role,
        OfferConstraints::RoleConstrints& roleConstraints,
        *constraints.mutable_role_constraints()) {
      vector<Group>& groups = expressions[role];
      
      for (OfferConstraints::RoleConstraints::Group& group :
           *roleConstraints.mutable_groups()) {
        ...
        
        groups.emplace_back(std::move(*evaluator));
      }
    }
    ```



src/master/offer_constraints_filter.cpp
Lines 244 (patched)
<https://reviews.apache.org/r/72741/#comment310632>

    Don't think we need to worry about the cost benefit of std moving the error message to
bother with the extra code? It will only be done once, and only in the error case.


- Benjamin Mahler


On Aug. 6, 2020, 4:24 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72741/
> -----------------------------------------------------------
> 
> (Updated Aug. 6, 2020, 4:24 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 a subclass of AgentResourcesFilter that suppoorts
> the Exists/NotExists offer constraints.
> 
> More constraints will be added to this filter in further patches.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt 4e15e3d99aa2cce2403fe07e762fef2fb4a27dea 
>   src/Makefile.am 447db323875e4cad46000977f4a61600baff8f89 
>   src/master/offer_constraints_filter.hpp PRE-CREATION 
>   src/master/offer_constraints_filter.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/72741/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>


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