mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrei Sekretenko <asekrete...@apache.org>
Subject Re: Review Request 72839: Added `OfferConstraints` to `struct Framework` in the master.
Date Fri, 11 Sep 2020 17:47:45 GMT


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


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