From reviews-return-92651-apmail-mesos-reviews-archive=mesos.apache.org@mesos.apache.org Wed Sep 23 19:21:28 2020 Return-Path: X-Original-To: apmail-mesos-reviews-archive@locus.apache.org Delivered-To: apmail-mesos-reviews-archive@locus.apache.org Received: from mxout1-ec2-va.apache.org (mxout1-ec2-va.apache.org [3.227.148.255]) by minotaur.apache.org (Postfix) with ESMTP id C92281A4FD for ; Wed, 23 Sep 2020 19:21:28 +0000 (UTC) Received: from mail.apache.org (mailroute1-lw-us.apache.org [207.244.88.153]) by mxout1-ec2-va.apache.org (ASF Mail Server at mxout1-ec2-va.apache.org) with SMTP id 8BB34435E4 for ; Wed, 23 Sep 2020 19:21:28 +0000 (UTC) Received: (qmail 73096 invoked by uid 500); 23 Sep 2020 19:21:28 -0000 Delivered-To: apmail-mesos-reviews-archive@mesos.apache.org Received: (qmail 73074 invoked by uid 500); 23 Sep 2020 19:21:28 -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 73052 invoked by uid 99); 23 Sep 2020 19:21:28 -0000 Received: from spamproc1-he-de.apache.org (HELO spamproc1-he-de.apache.org) (116.203.196.100) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 23 Sep 2020 19:21:28 +0000 Received: from localhost (localhost [127.0.0.1]) by spamproc1-he-de.apache.org (ASF Mail Server at spamproc1-he-de.apache.org) with ESMTP id 3CEC41FF39B for ; Wed, 23 Sep 2020 19:21:27 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamproc1-he-de.apache.org X-Spam-Flag: NO X-Spam-Score: 1.861 X-Spam-Level: * X-Spam-Status: No, score=1.861 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_NONE=0.001] autolearn=disabled Received: from mx1-he-de.apache.org ([116.203.227.195]) by localhost (spamproc1-he-de.apache.org [116.203.196.100]) (amavisd-new, port 10024) with ESMTP id eAxMZ5tGUcnk for ; Wed, 23 Sep 2020 19:21:26 +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 4C1477FB63 for ; Wed, 23 Sep 2020 19:21:26 +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 9107E1603F1; Wed, 23 Sep 2020 19:21:25 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============8621786638199517921==" MIME-Version: 1.0 Subject: Re: Review Request 72897: Allowed `OfferConstraintsFilter::create()` to return an empty `Result`. From: Benjamin Mahler To: Benjamin Mahler Cc: Andrei Sekretenko , mesos Date: Wed, 23 Sep 2020 19:21:25 -0000 Message-ID: <20200923192125.23297.99197@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/72897/ X-Sender: Benjamin Mahler References: <20200922205019.13989.95572@reviews-vm-he-fi.apache.org> In-Reply-To: <20200922205019.13989.95572@reviews-vm-he-fi.apache.org> Reply-To: Benjamin Mahler X-ReviewRequest-Repository: mesos --===============8621786638199517921== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit > On Sept. 22, 2020, 8:50 p.m., Benjamin Mahler wrote: > > Hm.. can you clarify what this gives us? > > Andrei Sekretenko wrote: > Now I think I need your input here. > If we decide to keep in a long term a guarantee that specifying default-constructed OfferConstraints is equivalent to not specifying any constraints at all, we are going to have a certain weirdness internally: right now, there are two functionally equivalent ways to specify to the allocator that the framework has **no offer constraints filtering**. > > One way is to set the `offerConstraintsFilter` in the framework's allocator options to `None`; another is to pass a filter constructed from empty constraints. > My intention was to eliminate one of these two ways. This patch gets rid of the second one (filters that are known to be a no-op are not created). > > However, on the second thought, just making a filter non-optional might make things simpler. > > I've benchmarked the default (no constraints) case and failed to find any difference between allocator's `struct Framework` always having a non-optional empty filter vs an empty Option. > No difference - despite of the fact that the first thing in the inner allocation loop is the agent filtering check `offerConstraintsFilter.isSome() && offerConstraintsFilter->isAgentExcluded(..)` (the version that makes filter non-optional drops `isSome()`). > > What do you think about just making the filter obligatory (and default-constructable, equivalent to constructing form default `OfferConstraints`)? > > Andrei Sekretenko wrote: > sorry, typo: s/constructing form/constructing from/ Removing the optionality sounds good, and was something I wondered back when seeing the `offerConstraintsFilter.isSome() && offerConstraintsFilter->isAgentExcluded(..)` logic. The only value I saw from optionality is us knowing (via endpoints) whether the framework is setting the field (and if not set, inferring that the framework is not constraint-capable). i.e. being able to distinguish between a framework that is not constraint capable vs a framework that is constraint capable but doesn't have any constraints to set and therefore uses an empty OfferConstraints. Thinking about it more though, I don't know that we would see a constraint-capable framework setting it to an empty value very often? Would we? I'm also not sure whether constraint-capable frameworks will set the field to empty vs not set it. If that's not done consistently then we wouldn't get much value from distinguishnig those cases in the endpoints. So, I think just removing optionality seems simplest! - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72897/#review221926 ----------------------------------------------------------- On Sept. 22, 2020, 6:05 p.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72897/ > ----------------------------------------------------------- > > (Updated Sept. 22, 2020, 6:05 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-10189 > https://issues.apache.org/jira/browse/MESOS-10189 > > > Repository: mesos > > > Description > ------- > > This patch makes it possible for the offer constraint filter creation > to return `None()` in cases when specified offer constraints would > result in creating a no-op `OfferConstraintsFilter` that would never > filter out anything, and actually implements this special handling > for an `OfferConstraints` message with an empty role constraints map. > > > Diffs > ----- > > include/mesos/allocator/allocator.hpp 6d67d5d5c32c1f18cdf6f925d747156c2bbc7820 > src/master/allocator/mesos/offer_constraints_filter.cpp 441ebc10219bf3bd623fac2bb08945ea9deb7ea3 > src/master/master.cpp fefa72d338384b6ccb1bcbbed7192713411035db > src/tests/master/offer_constraints_filter_tests.cpp f80d56c89d937b6a1b261202adefb37a1519bf0d > > > Diff: https://reviews.apache.org/r/72897/diff/1/ > > > Testing > ------- > > > Thanks, > > Andrei Sekretenko > > --===============8621786638199517921==--