> 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 > >
