From reviews-return-92568-apmail-mesos-reviews-archive=mesos.apache.org@mesos.apache.org Wed Sep 9 23:57:06 2020 Return-Path: X-Original-To: apmail-mesos-reviews-archive@locus.apache.org Delivered-To: apmail-mesos-reviews-archive@locus.apache.org Received: from mailroute1-lw-us.apache.org (mailroute1-lw-us.apache.org [207.244.88.153]) by minotaur.apache.org (Postfix) with ESMTP id 2D59319789 for ; Wed, 9 Sep 2020 23:57:06 +0000 (UTC) Received: from mail.apache.org (localhost [127.0.0.1]) by mailroute1-lw-us.apache.org (ASF Mail Server at mailroute1-lw-us.apache.org) with SMTP id D40BD1227E1 for ; Wed, 9 Sep 2020 23:57:05 +0000 (UTC) Received: (qmail 48164 invoked by uid 500); 9 Sep 2020 23:57:05 -0000 Delivered-To: apmail-mesos-reviews-archive@mesos.apache.org Received: (qmail 48136 invoked by uid 500); 9 Sep 2020 23:57:05 -0000 Mailing-List: contact reviews-help@mesos.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: reviews@mesos.apache.org Delivered-To: mailing list reviews@mesos.apache.org Received: (qmail 48122 invoked by uid 99); 9 Sep 2020 23:57:05 -0000 Received: from pnap-us-west-generic-nat.apache.org (HELO spamd1-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 09 Sep 2020 23:57:05 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd1-us-west.apache.org (ASF Mail Server at spamd1-us-west.apache.org) with ESMTP id 7859DC03A6 for ; Wed, 9 Sep 2020 23:57:04 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd1-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: 1.862 X-Spam-Level: * X-Spam-Status: No, score=1.862 tagged_above=-999 required=6.31 tests=[HTML_MESSAGE=0.2, KAM_DMARC_STATUS=0.01, KAM_LAZY_DOMAIN_SECURITY=1, KAM_LOTSOFHASH=0.25, KHOP_HELO_FCRDNS=0.4, SPF_HELO_NONE=0.001, SPF_NONE=0.001] autolearn=disabled Received: from mx1-he-de.apache.org ([10.40.0.8]) by localhost (spamd1-us-west.apache.org [10.40.0.7]) (amavisd-new, port 10024) with ESMTP id xGDeJMB600b7 for ; Wed, 9 Sep 2020 23:57:02 +0000 (UTC) Received-SPF: None (mailfrom) identity=mailfrom; client-ip=95.217.165.199; helo=reviews-vm-he-fi.apache.org; envelope-from=noreply@reviews.apache.org; receiver= Received: from reviews-vm-he-fi.apache.org (static.199.165.217.95.clients.your-server.de [95.217.165.199]) by mx1-he-de.apache.org (ASF Mail Server at mx1-he-de.apache.org) with ESMTP id 5A33B7F5C7 for ; Wed, 9 Sep 2020 23:57:02 +0000 (UTC) Received: from reviews-vm-he-fi.apache.org (reviews-vm-he-fi.apache.org [127.0.0.1]) by reviews-vm-he-fi.apache.org (Postfix) with ESMTP id BC2771603E3; Wed, 9 Sep 2020 23:57:01 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============2269292942748696716==" MIME-Version: 1.0 Subject: Re: Review Request 72839: Added `OfferConstraints` to `struct Framework` in the master. From: Benjamin Mahler To: Benjamin Mahler Cc: Andrei Sekretenko , mesos Date: Wed, 09 Sep 2020 23:57:01 -0000 Message-ID: <20200909235701.13989.2433@reviews-vm-he-fi.apache.org> X-ReviewBoard-URL: https://reviews.apache.org/ Auto-Submitted: auto-generated Sender: Benjamin Mahler X-ReviewGroup: mesos X-Auto-Response-Suppress: DR, RN, OOF, AutoReply X-ReviewRequest-URL: https://reviews.apache.org/r/72839/ X-Sender: Benjamin Mahler X-ReviewBoard-ShipIt: 1 References: <20200907152630.13941.37670@reviews-vm-he-fi.apache.org> In-Reply-To: <20200907152630.13941.37670@reviews-vm-he-fi.apache.org> Reply-To: Benjamin Mahler X-ReviewRequest-Repository: mesos --===============2269292942748696716== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72839/#review221836 ----------------------------------------------------------- Ship it! 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? src/master/master.hpp Lines 2547 (patched) { on newline src/master/master.cpp Lines 3261-3275 (original), 3297-3313 (patched) Maybe just touch the protobuf once, then work off the option? Looks a bit clearer? ``` Option 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 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); } ``` - Benjamin Mahler On Sept. 7, 2020, 3: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, 3: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 3d1d4723d1bcef1880404007410bf00656399f10 > src/master/master.cpp 5b5d5c359ca48eb0b4f5554fe7c91e928d4da08d > > > Diff: https://reviews.apache.org/r/72839/diff/1/ > > > Testing > ------- > > > Thanks, > > Andrei Sekretenko > > --===============2269292942748696716==--