mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrei Sekretenko <asekrete...@mesosphere.io>
Subject Re: Review Request 70379: Added validation that the principal stays the same on resubscription.
Date Fri, 05 Apr 2019 15:13:36 GMT


> On April 4, 2019, 9:53 p.m., Gastón Kleiman wrote:
> > src/master/master.cpp
> > Lines 2558-2560 (patched)
> > <https://reviews.apache.org/r/70379/diff/1/?file=2137245#file2137245line2558>
> >
> >     We use stout's `stringify()` for this, which delegates to the corresponding
`operator<<` if one exists.

Implementing `operator<<(const Option<T>&)` without breaking `UniversalPrint`-related
magic in googletest turns out to be a somewhat nontrivial task wich does not fit into this
ticket.

Also, the proper output of such operator will definitely require some discussion.

So, regarding your previous comment, I decided to drop this altogether.


> On April 4, 2019, 9:53 p.m., Gastón Kleiman wrote:
> > src/master/master.cpp
> > Lines 2562-2563 (patched)
> > <https://reviews.apache.org/r/70379/diff/1/?file=2137245#file2137245line2562>
> >
> >     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)`.

After running a diff against the two Master::subscribe() methods I relaized that they have
identical (except for comments) validation part. After refactoring this away (it will be the
review PRECEDING the review with the tests) this method becomes unneeded.


> On April 4, 2019, 9:53 p.m., Gastón Kleiman wrote:
> > src/master/master.cpp
> > Lines 2570 (patched)
> > <https://reviews.apache.org/r/70379/diff/1/?file=2137245#file2137245line2570>
> >
> >     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());`

I have no strong opinion on the "almost always/never auto" debate. Changing this from `const
auto*` to `const Framework*`.


> On April 4, 2019, 9:53 p.m., Gastón Kleiman wrote:
> > src/master/master.cpp
> > Lines 2592-2596 (patched)
> > <https://reviews.apache.org/r/70379/diff/1/?file=2137245#file2137245line2592>
> >
> >     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.

The only reason I added this logging is that there will be no information about the old and
the new principals in the validation error message.

Having such information on hands would be nice for the guys who will run into a buggy framework.

If there already is a more or less convenient way for the operator to extract the information
about the old principal from master, I'll add the new principal to the error and gladly drop
this line.


- Andrei


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70379/#review214382
-----------------------------------------------------------


On April 5, 2019, 3:12 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70379/
> -----------------------------------------------------------
> 
> (Updated April 5, 2019, 3:12 p.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 resubscription.
> 
> 
> Diffs
> -----
> 
>   src/master/master.cpp cf5caa0893ba1387a1f3a9d129ecd7d974f776bd 
> 
> 
> Diff: https://reviews.apache.org/r/70379/diff/2/
> 
> 
> Testing
> -------
> 
> make check
> 
> Now the two failing tests from https://reviews.apache.org/r/70377/ pass.
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message