> On Sept. 10, 2020, 1:57 a.m., Benjamin Mahler wrote: > > Hm.. I was wondering how we exposed the suppressed roles in /frameworks & > > GET_FRAMEWORKS, since I didn't see them in the interfaces here, but looks > > like we don't expose them? > > > > An alternative would be to include this in the allocatorOptions, like > > suppressed roles, but store allocatorOptions in the master's Framework > > struct, rather than throwing it away after passing it through to the > > allocator. Seems a bit cleaner and won't require suppressed roles to also > > get added to all these interfaces? I guess you perhaps didn't go this route > > since allocator options is storing the compiled form of this information, > > and the allocator does not need the raw form?
I went this route for a number of reasons: - allocator does not need the raw form of the offer constraints - neither does master normally need the filter (only when handling the debug endpoint, that is, once in eternity) - filter is non-copyable; keeping it this way would greatly simplify things should performance reasons at some later point require a piece of a mutable state in the filter (intermediate result caching, constraint reordering, whatever) > On Sept. 10, 2020, 1:57 a.m., Benjamin Mahler wrote: > > src/master/master.cpp > > Lines 3261-3275 (original), 3297-3313 (patched) > > <https://reviews.apache.org/r/72839/diff/1/?file=2239295#file2239295line3297> > > > > Maybe just touch the protobuf once, then work off the option? Looks a > > bit clearer? > > > > ``` > > Option<OfferConstraints> offerConstraints; > > if (call.has_offer_constraints()) { > > offerConstraints = std::move(*call.mutable_offer_constraints()); > > } > > > > allocator::FrameworkOptions allocatorOptions; > > if (offerConstraints.isSome()) { > > // TODO(asekretenko): Validate roles in offer constraints (see > > MESOS-10176). > > Try<OfferConstraintsFilter> filter = OfferConstraintsFilter::create( > > offerConstraintsFilterOptions, *offerConstraints); > > > > if (filter.isError()) { > > return process::http::BadRequest( > > "'UpdateFramework.offer_constraints' are not valid: " + > > filter.error()); > > } > > > > allocatorOptions.offerConstraintsFilter = std::move(*filter); > > } > > ``` agree, looks slightly clearer - Andrei ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72839/#review221836 ----------------------------------------------------------- On Sept. 7, 2020, 5:26 p.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72839/ > ----------------------------------------------------------- > > (Updated Sept. 7, 2020, 5:26 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-10179 > https://issues.apache.org/jira/browse/MESOS-10179 > > > Repository: mesos > > > Description > ------- > > This is a prerequisite for exposing framework's offer constraints > via master API endpoints. > > > Diffs > ----- > > src/master/framework.cpp e9e2d20bdc89dd299b2376dd2f67ae056538c3e5 > src/master/master.hpp 8c2f56a0707eed8f61147715497b64a119bb02d2 > src/master/master.cpp 438ef98a96013ce378956ed5a0ad96d0dbb4469c > > > Diff: https://reviews.apache.org/r/72839/diff/2/ > > > Testing > ------- > > > Thanks, > > Andrei Sekretenko > >
