From reviews-return-92235-apmail-mesos-reviews-archive=mesos.apache.org@mesos.apache.org Thu Aug 13 21:23:07 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 0022719424 for ; Thu, 13 Aug 2020 21:23: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 9E1B7125F36 for ; Thu, 13 Aug 2020 21:23:06 +0000 (UTC) Received: (qmail 30286 invoked by uid 500); 13 Aug 2020 21:23:06 -0000 Delivered-To: apmail-mesos-reviews-archive@mesos.apache.org Received: (qmail 30265 invoked by uid 500); 13 Aug 2020 21:23:06 -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 30252 invoked by uid 99); 13 Aug 2020 21:23:06 -0000 Received: from pnap-us-west-generic-nat.apache.org (HELO spamd3-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 13 Aug 2020 21:23:06 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd3-us-west.apache.org (ASF Mail Server at spamd3-us-west.apache.org) with ESMTP id 52EB918144E for ; Thu, 13 Aug 2020 21:23:05 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd3-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: 2.214 X-Spam-Level: ** X-Spam-Status: No, score=2.214 tagged_above=-999 required=6.31 tests=[HTML_MESSAGE=0.2, KAM_DMARC_STATUS=0.01, KAM_LAZY_DOMAIN_SECURITY=1, KHOP_HELO_FCRDNS=0.001, MANY_SPAN_IN_TEXT=1, SPF_HELO_NONE=0.001, SPF_NONE=0.001, URIBL_BLOCKED=0.001] autolearn=disabled Received: from mx1-ec2-va.apache.org ([10.40.0.8]) by localhost (spamd3-us-west.apache.org [10.40.0.10]) (amavisd-new, port 10024) with ESMTP id GWfEvj_4oTEm for ; Thu, 13 Aug 2020 21:23:01 +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-ec2-va.apache.org (ASF Mail Server at mx1-ec2-va.apache.org) with ESMTP id 93C83BE390 for ; Thu, 13 Aug 2020 21:23:00 +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 70DF8160197; Thu, 13 Aug 2020 21:22:59 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============9066017291244886404==" MIME-Version: 1.0 Subject: Re: Review Request 72741: Implemented offer constraints filter with Exists/NotExists predicates. From: Benjamin Mahler To: Benjamin Mahler Cc: Andrei Sekretenko , mesos Date: Thu, 13 Aug 2020 21:22:59 -0000 Message-ID: <20200813212259.6277.20393@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/72741/ X-Sender: Benjamin Mahler References: <20200806162418.14033.5643@reviews-vm-he-fi.apache.org> In-Reply-To: <20200806162418.14033.5643@reviews-vm-he-fi.apache.org> X-ReviewBoard-Diff-For: src/master/offer_constraints_filter.cpp X-ReviewBoard-Diff-For: src/master/offer_constraints_filter.hpp Reply-To: Benjamin Mahler X-ReviewRequest-Repository: mesos --===============9066017291244886404== 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/72741/#review221571 ----------------------------------------------------------- src/master/offer_constraints_filter.cpp Lines 43-56 (patched) We should check that they aren't both set? src/master/offer_constraints_filter.cpp Lines 44-51 (patched) Can we use a switch here per our approach to protobuf enum handling? That gives us the benefit of a compiler error when we miss a case (now or later) src/master/offer_constraints_filter.cpp Lines 59-60 (patched) Use single quotes here, and ideally a clear field path: Exactly one of 'AttributeConstraint::Selector::name' of 'AttributeConstraint::Selector::pseudoattribute_type' must be set src/master/offer_constraints_filter.cpp Lines 64 (patched) This is a little confusing to me since we already have a predicate type from the protobuf and this is essentially the same thing but in a more usable format for this code? I wonder if there's a way to accomplish what we want without the extra predicate type. Since the evaluator holds one of these predicates, maybe the evaluator can be the one to hold any extra predicate logic / information needed rather than having a duplicate predicate type. src/master/offer_constraints_filter.cpp Lines 121 (patched) braces on the next line (looks like some other classes functions need to be updated in this file) src/master/offer_constraints_filter.cpp Lines 134-143 (patched) I think we allow multiple entries for the same attribute key, on the agent side? Not sure if anyone uses that. src/master/offer_constraints_filter.cpp Lines 147 (patched) switch ( src/master/offer_constraints_filter.cpp Lines 181 (patched) Ditto, don't think we need to bother with the std move here? src/master/offer_constraints_filter.cpp Lines 192-195 (patched) I think we generally put the public interface first, followed by an explicit private section? Ditto in the other case(s) above. src/master/offer_constraints_filter.cpp Lines 237 (patched) hm.. maybe do the role insertion above this loop and have a reference to the vector? Seems cleaner and cheaper? ``` foreachpair ( const string& role, OfferConstraints::RoleConstrints& roleConstraints, *constraints.mutable_role_constraints()) { vector& groups = expressions[role]; for (OfferConstraints::RoleConstraints::Group& group : *roleConstraints.mutable_groups()) { ... groups.emplace_back(std::move(*evaluator)); } } ``` src/master/offer_constraints_filter.cpp Lines 244 (patched) Don't think we need to worry about the cost benefit of std moving the error message to bother with the extra code? It will only be done once, and only in the error case. - Benjamin Mahler On Aug. 6, 2020, 4:24 p.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72741/ > ----------------------------------------------------------- > > (Updated Aug. 6, 2020, 4:24 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 implements a subclass of AgentResourcesFilter that suppoorts > the Exists/NotExists offer constraints. > > More constraints will be added to this filter in further patches. > > > Diffs > ----- > > src/CMakeLists.txt 4e15e3d99aa2cce2403fe07e762fef2fb4a27dea > src/Makefile.am 447db323875e4cad46000977f4a61600baff8f89 > src/master/offer_constraints_filter.hpp PRE-CREATION > src/master/offer_constraints_filter.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/72741/diff/1/ > > > Testing > ------- > > > Thanks, > > Andrei Sekretenko > > --===============9066017291244886404==--