From reviews-return-92545-apmail-mesos-reviews-archive=mesos.apache.org@mesos.apache.org Tue Sep 8 22:40:21 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 467991A532 for ; Tue, 8 Sep 2020 22:40:21 +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 00FEE428AF for ; Tue, 8 Sep 2020 22:40:21 +0000 (UTC) Received: (qmail 66450 invoked by uid 500); 8 Sep 2020 22:40:20 -0000 Delivered-To: apmail-mesos-reviews-archive@mesos.apache.org Received: (qmail 66419 invoked by uid 500); 8 Sep 2020 22:40:20 -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 66402 invoked by uid 99); 8 Sep 2020 22:40:20 -0000 Received: from pnap-us-west-generic-nat.apache.org (HELO spamd2-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 08 Sep 2020 22:40:20 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd2-us-west.apache.org (ASF Mail Server at spamd2-us-west.apache.org) with ESMTP id A5D541A3461 for ; Tue, 8 Sep 2020 22:40:19 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd2-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: 1.612 X-Spam-Level: * X-Spam-Status: No, score=1.612 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.4, SPF_HELO_NONE=0.001, SPF_NONE=0.001] autolearn=disabled Received: from mx1-ec2-va.apache.org ([10.40.0.8]) by localhost (spamd2-us-west.apache.org [10.40.0.9]) (amavisd-new, port 10024) with ESMTP id eaWYiW5HqD_6 for ; Tue, 8 Sep 2020 22:40:18 +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 F29D5BB9A5 for ; Tue, 8 Sep 2020 22:40:17 +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 C1F851603D9; Tue, 8 Sep 2020 22:40:16 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============7585703114347952632==" MIME-Version: 1.0 Subject: Re: Review Request 72838: Moved framework update out of `connectAndActivateRecoveredFramework`. From: Benjamin Mahler To: Benjamin Mahler Cc: Andrei Sekretenko , mesos Date: Tue, 08 Sep 2020 22:40:16 -0000 Message-ID: <20200908224016.13989.48214@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/72838/ X-Sender: Benjamin Mahler X-ReviewBoard-ShipIt: 1 References: <20200907152439.13941.40617@reviews-vm-he-fi.apache.org> In-Reply-To: <20200907152439.13941.40617@reviews-vm-he-fi.apache.org> Reply-To: Benjamin Mahler X-ReviewRequest-Repository: mesos --===============7585703114347952632== 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/72838/#review221818 ----------------------------------------------------------- Ship it! src/master/master.cpp Lines 3130-3137 (original), 3120-3127 (patched) Not really related to this change, but I got pretty confused here thinking about 'force'. One thing I was trying to figure out was: if a scheduler failed over, and the old scheduler instance is disconnected, I would expect that you don't need 'force == true' for the new instance to connect. But it looks like this check would disallow the new scheduler instance and always requires `force == true` for a new scheduler instance even if the old one disconnected already. Reading the comment on `force` in the v0 protobuf, I now realize that it's only used by the driver and the driver always sets it to true whenever it registers after failover. For v1 http, looks like it will always wind up as `force == false` during the devolve call, but we don't even look at `force` in that `_subscribe` path. Some suggestions: (1) Looks like this: // Using the "force" field of the scheduler allows us to keep a Should be s/keep/reject/ ? That's the case we want to disallow from coming back. (2) Perhaps we clarify here that the v0 driver always sets `force` to true when it registers for the first time post failover? Otherwise it leaves it false to ensure the case described is rejected. src/master/master.cpp Line 3137 (original), 3127 (patched) since you're touching this line anyway, feel free to remove the unnecessary parens - Benjamin Mahler On Sept. 7, 2020, 3:24 p.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72838/ > ----------------------------------------------------------- > > (Updated Sept. 7, 2020, 3:24 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-10179 > https://issues.apache.org/jira/browse/MESOS-10179 > > > Repository: mesos > > > Description > ------- > > This patch consolidates updating framework in the Subscribe call > by moving `updateFramework()` invocation from > `connectAndActivateRecoveredFramework()` into `Master::_subscribe()`. > > > Diffs > ----- > > src/master/master.hpp 3d1d4723d1bcef1880404007410bf00656399f10 > src/master/master.cpp 5b5d5c359ca48eb0b4f5554fe7c91e928d4da08d > > > Diff: https://reviews.apache.org/r/72838/diff/1/ > > > Testing > ------- > > > Thanks, > > Andrei Sekretenko > > --===============7585703114347952632==--