> On Aug. 11, 2020, 8:55 p.m., Benjamin Mahler wrote:
> > include/mesos/allocator/allocator.hpp
> > Lines 72-96 (patched)
> > <https://reviews.apache.org/r/72740/diff/1/?file=2237310#file2237310line72>
> >
> >     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.

I think I'll reconsider using an abstract class here (and will replace it with pImpl-based `OfferConstraintsFilter`).

This object is only ever going to be constructed in the scheduler API code in the `Master`, so the only possible use of an abstract inteface could be mocking the filter object in tests. 
However, given that the filter implements a pure fucntion (or, in future, it might implement a set of pure functions consistent with one another), I don't see any real benefits from tests using a filter mock. 
Moreover, mock-based filter tests might hinder such things as implementing caching in the allocator, should we eventually need that.

Why not `std::function<bool(const string&, const AgentInfo&)>`:

1. (major issue) If we add resource-based constraints, we will likely need to provide several filtering methods consistent with one another. 
  `isAgentExcluded(const string& role, const SlaveInfo&)`, although likely still necessary (benchmarking attempts clearly show that agents should be filtered as early as possible), will no longer be sufficient; we will have to add `isExcluded(const string& role, const SlaveInfo&, const Resources&)`
  which will have to evaluate the same **attribute** constraints to apply proper resource constraints. Passing two functons and requiring that they behave consistently is not a sane thing.

2. (minor issue) std::function needs to be copyable. There are no real benefits from having the filter copyable, but there are potential benefits from having a copy disabled right from the start: should we eventually need an internal cache in the filter, a non-copyable filter will make things simpler.


Nevertheless, we need a layer of indirection: otherwise, the whole definition of the filter class and all its internal structures (+ a couple of RE2 headers) get indirectly included (almost) everywhere. 
Looks like pImpl is a way to go.


- Andrei


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


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