From reviews-return-92303-apmail-mesos-reviews-archive=mesos.apache.org@mesos.apache.org Tue Aug 18 18:36:57 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 46E501AAF0 for ; Tue, 18 Aug 2020 18:36:57 +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 869DE1252D3 for ; Tue, 18 Aug 2020 18:36:56 +0000 (UTC) Received: (qmail 17579 invoked by uid 500); 18 Aug 2020 18:36:55 -0000 Delivered-To: apmail-mesos-reviews-archive@mesos.apache.org Received: (qmail 17556 invoked by uid 500); 18 Aug 2020 18:36:55 -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 17339 invoked by uid 99); 18 Aug 2020 18:36:54 -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; Tue, 18 Aug 2020 18:36:54 +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 1A055C0181 for ; Tue, 18 Aug 2020 18:36:54 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd1-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: 1.611 X-Spam-Level: * X-Spam-Status: No, score=1.611 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.398, SPF_HELO_NONE=0.001, SPF_NONE=0.001, URIBL_BLOCKED=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 kmetcv5rWKzN for ; Tue, 18 Aug 2020 18:36:52 +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 1B0AA7FB86 for ; Tue, 18 Aug 2020 18:36:52 +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 7E70D160ECC; Tue, 18 Aug 2020 18:36:51 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============0677309678288807582==" MIME-Version: 1.0 Subject: Re: Review Request 72742: Added basic tests for the `OfferConstraintsFilter`. From: Benjamin Mahler To: Benjamin Mahler Cc: Andrei Sekretenko , mesos Date: Tue, 18 Aug 2020 18:36:51 -0000 Message-ID: <20200818183651.26190.44045@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/72742/ X-Sender: Benjamin Mahler References: <20200814165956.14498.17414@reviews-vm-he-fi.apache.org> In-Reply-To: <20200814165956.14498.17414@reviews-vm-he-fi.apache.org> X-ReviewBoard-Diff-For: src/tests/master/offer_constraints_filter_tests.cpp Reply-To: Benjamin Mahler X-ReviewRequest-Repository: mesos --===============0677309678288807582== 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/72742/#review221625 ----------------------------------------------------------- Great to have unit tests for this! Mainly left a suggestion about using a JSON approach to build the protobuf. Do we want to add another test here for attributes with multiple entries? (i.e. showing that we only look at the first entry) src/tests/master/offer_constraints_filter_tests.cpp Lines 72-76 (patched) Hm.. see my other comment below, might be able to avoid the `singleAttributeConstraintFilter` by using the same JSON approach to set up the protobuf in all the tests? It would also be nice to have all the OfferConstraintsFilter construction consistent (rather than some going through a helper and some not). src/tests/master/offer_constraints_filter_tests.cpp Lines 196-211 (patched) For this and the others above, it's often pretty hard for a reader to parse through what the protobuf is with setting it up in C++, is it possible to just do a json conversion here as we've done in some other places? ``` Try json = JSON::parse( R"~( { "role_constraints": { "role1": { "attribute_constraints": [{ "selector": {"attribute_name": "foo"}, "predicate": {"exists": {}} }] }, "role2": { "attribute_constraints": [{ "selector": {"attribute_name": "bar"}, "predicate": {"not_exists": {}} }] } } })~"); ASSERT_SOME(json); Try constraints = protobuf::parse(*json); ASSERT_SOME(constraints); ``` This makes it really easy on the reader to see what the constraints look like. - Benjamin Mahler On Aug. 14, 2020, 4:59 p.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72742/ > ----------------------------------------------------------- > > (Updated Aug. 14, 2020, 4:59 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-10171 > https://issues.apache.org/jira/browse/MESOS-10171 > > > Repository: mesos > > > Description > ------- > > Added basic tests for the `OfferConstraintsFilter`. > > > Diffs > ----- > > src/Makefile.am 447db323875e4cad46000977f4a61600baff8f89 > src/tests/CMakeLists.txt cf579f8d6e5176e0d4eadabcbadcbb99aa6c737e > src/tests/master/offer_constraints_filter_tests.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/72742/diff/3/ > > > Testing > ------- > > > Thanks, > > Andrei Sekretenko > > --===============0677309678288807582==--