From reviews-return-92515-apmail-mesos-reviews-archive=mesos.apache.org@mesos.apache.org Wed Sep 2 23:31:26 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 9A7881AFBA for ; Wed, 2 Sep 2020 23:31:26 +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 481A5121B02 for ; Wed, 2 Sep 2020 23:31:26 +0000 (UTC) Received: (qmail 96089 invoked by uid 500); 2 Sep 2020 23:31:26 -0000 Delivered-To: apmail-mesos-reviews-archive@mesos.apache.org Received: (qmail 96054 invoked by uid 500); 2 Sep 2020 23:31:26 -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 96031 invoked by uid 99); 2 Sep 2020 23:31:25 -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; Wed, 02 Sep 2020 23:31:25 +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 F3C1E181449 for ; Wed, 2 Sep 2020 23:31:24 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd3-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.399, 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 (spamd3-us-west.apache.org [10.40.0.10]) (amavisd-new, port 10024) with ESMTP id sU7PSrs_OS0b for ; Wed, 2 Sep 2020 23:31:23 +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 E30CB7F98B for ; Wed, 2 Sep 2020 23:31:22 +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 4C9A61603B4; Wed, 2 Sep 2020 23:31:22 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============8865885924582272202==" MIME-Version: 1.0 Subject: Re: Review Request 72833: Removed `bool operator==(const FrameworkInfo&, const FrameworkInfo&)`. From: Benjamin Mahler To: Benjamin Mahler Cc: Andrei Sekretenko , mesos Date: Wed, 02 Sep 2020 23:31:22 -0000 Message-ID: <20200902233122.2546.51313@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/72833/ X-Sender: Benjamin Mahler X-ReviewBoard-ShipIt: 1 References: <20200901194514.2546.39939@reviews-vm-he-fi.apache.org> In-Reply-To: <20200901194514.2546.39939@reviews-vm-he-fi.apache.org> Reply-To: Benjamin Mahler X-ReviewRequest-Repository: mesos --===============8865885924582272202== 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/72833/#review221787 ----------------------------------------------------------- Fix it, then Ship it! include/mesos/type_utils.hpp Lines 119-122 (original) Hm.. Can we actually leave these but as `delete`d functions? Seems like C++ does allow non-member functions to be deleted? https://abseil.io/tips/143 https://stackoverflow.com/questions/42332777/what-is-the-point-of-using-delete-on-a-non-member-function ``` // This has been deleted in favor of ... inline bool operator==(const FrameworkInfo& left, const FrameworkInfo& right) = delete; ``` That will make it a lot less likely that someone re-adds this operator when they find it's missing. Also helps them find the equivalent function. src/tests/api_tests.cpp Lines 3354-3358 (patched) Should we just have DEFAULT_FRAMEWORK_INFO explicitly set it to false? Just a thought.. src/tests/master_allocator_tests.cpp Lines 374 (patched) Not sure that this test needs to be concerned about checking the framework info equality on addFramework calls, it could just check the framework id matches, but even that doesn't seem necessary for this test? Especially since not checking or checking just the ID wouldn't require a custom matcher. - Benjamin Mahler On Sept. 1, 2020, 7:45 p.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72833/ > ----------------------------------------------------------- > > (Updated Sept. 1, 2020, 7:45 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-10166 > https://issues.apache.org/jira/browse/MESOS-10166 > > > Repository: mesos > > > Description > ------- > > This patch removes the outdated `operator==` for `FrameworkInfo` > (which has been comparing only `name` and `user` fields) > and replaces it with `equivalent()` in the tests that need to compare > `FrameworkInfo`s. > > > Diffs > ----- > > include/mesos/type_utils.hpp da9fd9694b08827b968514b4b078097c912640c8 > include/mesos/v1/mesos.hpp d8304f10d6ac6bbcf014e515e0bfd77a2b96e138 > src/tests/api_tests.cpp e0ce0335bf99f1f624efcca8c86f797df472d68f > src/tests/master_allocator_tests.cpp 416b7ba733c3a9fc75e64fecf088ff13548bab3f > src/tests/resources_tests.cpp bd044310e297174155b88d5694641acd5df4cf59 > > > Diff: https://reviews.apache.org/r/72833/diff/1/ > > > Testing > ------- > > `support/mesos-gtest-runner.py src/mesos-tests` > > > Thanks, > > Andrei Sekretenko > > --===============8865885924582272202==--