----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70379/#review214382 -----------------------------------------------------------
src/master/master.hpp Lines 1194-1195 (patched) <https://reviews.apache.org/r/70379/#comment300573> I'd write something like this: ``` Validates that the principal provided on resubscription matches the one already known by the master. ``` src/master/master.cpp Lines 2525-2527 (patched) <https://reviews.apache.org/r/70379/#comment300575> We tend to prefer inlining statements like this one instead of creating static methods. src/master/master.cpp Lines 2558-2560 (patched) <https://reviews.apache.org/r/70379/#comment300574> We use stout's `stringify()` for this, which delegates to the corresponding `operator<<` if one exists. src/master/master.cpp Lines 2562-2563 (patched) <https://reviews.apache.org/r/70379/#comment300589> I would follow the current pattern in `Master::subscribe` and instead of adding this method add one with this signature: `bool Master::frameworkPrincipalChanged(const FrameworkInfo& frameworkInfo)`. src/master/master.cpp Lines 2570 (patched) <https://reviews.apache.org/r/70379/#comment300577> we tend to use `auto` only for complex types or inside `foreach` statements. To stay consistent with what's normally used, I would do: `Framework* framework = getFramework(frameworkInfo.id());` src/master/master.cpp Lines 2573-2574 (patched) <https://reviews.apache.org/r/70379/#comment300580> I would change this to: ``` // TODO(asekretenko): Masters don't store `FrameworkInfo` messages in the replicated log, so it is possible that the previous principal is still unknown at the time of re-registration. This has to be changed if we decide to start storing `FrameworkInfo` messages. ``` And add an empty line before the TODO. src/master/master.cpp Lines 2592-2596 (patched) <https://reviews.apache.org/r/70379/#comment300584> We don't end lines with the `<<` operator. We start new lines with it instead, so this would look like: ``` LOG(WARNING) << "Framework " << frameworkInfo.id() << " which had a principal '" << oldPrincipal << "' tried to resubscribe with a new principal '" << newPrincipal << "'"; ``` However we don't have to log anything here, because `Master::subscribe()` already logs validation errors. src/master/master.cpp Lines 2598 (patched) <https://reviews.apache.org/r/70379/#comment300593> I think I'd rather not mention the old principal to prevent leaking information. src/master/master.cpp Lines 10763 (patched) <https://reviews.apache.org/r/70379/#comment300591> Looks like this variable is not used. - Gastón Kleiman On April 3, 2019, 9:04 a.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70379/ > ----------------------------------------------------------- > > (Updated April 3, 2019, 9:04 a.m.) > > > Review request for mesos and Gastón Kleiman. > > > Bugs: MESOS-2842 > https://issues.apache.org/jira/browse/MESOS-2842 > > > Repository: mesos > > > Description > ------- > > Added validation that the principal stays the same on a framework > resubscription. > This solves MESOS-2842. > > There are at least two objectionable places in this patch - see TODO in the > diff. > > > Diffs > ----- > > src/master/master.hpp 94891af9deeaddb3333fc9d6eabb243aed97f7b7 > src/master/master.cpp cf5caa0893ba1387a1f3a9d129ecd7d974f776bd > > > Diff: https://reviews.apache.org/r/70379/diff/1/ > > > Testing > ------- > > make check > > Now the two failing tests from https://reviews.apache.org/r/70377/ pass. > > > Thanks, > > Andrei Sekretenko > >
