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 72740: Introduced an interface for plugging offer filtering into the allocator.
Date Tue, 11 Aug 2020 20:55:50 GMT

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




include/mesos/allocator/allocator.hpp
Lines 72-96 (patched)
<https://reviews.apache.org/r/72740/#comment310617>

    Hm.. `AgentResourcesFilter` as a name looks a bit unintuitive since resources are not
involved and I would probably think it's about including or excluding resources within an
agent or something, instead of dealing with agent level inclusion / exclusion using the agent
metadata.
    
    I must be missing something obvious here, but why do we need the abstract interface? Is
it for testing?
    
    I also wonder whether we can just call it `OfferConstrainsFilter` to just be more direct,
and then say that we need it to be abstract for testing purposes (assuming that's why).
    
    The last thought is on why we need the class, will it be holding state? Otherwise, seems
like this could just be a function object? I can see that potentially being very messy though
when defining variables / passing it around.



include/mesos/allocator/allocator.hpp
Lines 109-117 (patched)
<https://reviews.apache.org/r/72740/#comment310618>

    Wondering why we need the abstract class / unique_ptr noise added here, see comment above.
    
    Although I wonder, even if we needed an interface for it, perhaps we could hide the pointer
indirection inside of the class using the Pimpl pattern and just std::move'ing it or something?
Just a thought..



src/master/allocator/mesos/hierarchical.cpp
Lines 2394-2398 (patched)
<https://reviews.apache.org/r/72740/#comment310616>

    Hm.. the first thing that jumps out at me here is that `agentResourcesFilter` seems a
bit odd since there are no resources involved. Perhaps `framework.agentFilter.isExcluded()`
(since a filter is either including or excluding which I always found a bit unclear) or `framework.isAgentFiltered(role,
slave.info)`?
    
    I also wonder about the abstract class indirection, see above.


- Benjamin Mahler


On Aug. 6, 2020, 4:30 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72740/
> -----------------------------------------------------------
> 
> (Updated Aug. 6, 2020, 4:30 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 introduces an abstract class for a pluggable object acting
> as a filter for agent resources to be offered to a framework's role,
> adds this object into FrameworkOptions and wires using this object
> into the allocator.
> 
> 
> Diffs
> -----
> 
>   include/mesos/allocator/allocator.hpp c700528e14bb42f6cea37f42dd7b72eb76a1a6b9 
>   src/master/allocator/mesos/hierarchical.hpp e444e470eb085cea167f84f8540d1769d662c222

>   src/master/allocator/mesos/hierarchical.cpp 9e5079942263132d09c6bd9abbdc8858cd2ef138

>   src/master/master.cpp 6a013e334b19bd108791d1c5fd0864c710aae8cb 
> 
> 
> Diff: https://reviews.apache.org/r/72740/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>


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