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