----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70666/#review215375 -----------------------------------------------------------
Fix it, then Ship it! Looks good, but some clarity in the description and some TODOs indicating that this does not validate all `user` and `checkpoint` would be great. src/master/master.cpp Line 2574 (original), 2574 (patched) <https://reviews.apache.org/r/70666/#comment302060> This is not accurate, if the framework attempts to change 'user' or 'checkpoint', which are immutable, this validation doesn't catch it. Can you comment on the intention here more clearly? I see https://reviews.apache.org/r/70669/ updates it later to be more coherent: so, some appropriate TODOs here are needed to indicate where this is going. src/master/master.cpp Line 2585 (original), 2585 (patched) <https://reviews.apache.org/r/70666/#comment302056> newline and space before the { Not yours, I realize it's copied from the condition above, but probably the following is more readable? ``` if (validationError.isSome()) { ``` i.e. "if there is an error" instead of "if there isn't no error" src/master/validation.hpp Lines 115 (patched) <https://reviews.apache.org/r/70666/#comment302062> Ditto, a TODO here is needed to indicate that this doesn't actually check all immutable fields. src/master/validation.cpp Lines 574-582 (patched) <https://reviews.apache.org/r/70666/#comment302054> no need for the std:: prefix anymore src/master/validation.cpp Lines 576 (patched) <https://reviews.apache.org/r/70666/#comment302052> not yours, but 2 space indent here src/master/validation.cpp Lines 579 (patched) <https://reviews.apache.org/r/70666/#comment302057> Perhaps the following for a bit more clarity: ``` Option<string> newPrincipal = None(); ``` src/master/validation.cpp Lines 590-594 (patched) <https://reviews.apache.org/r/70666/#comment302055> not yours, but just a comment, we usually try to put the opening and closing single quotes on the same line if possible, to reduce accidental omission of the closing quote (which has happened quite often) src/master/validation.cpp Lines 591 (patched) <https://reviews.apache.org/r/70666/#comment302058> Not yours, but we can do the following to clean it up a bit: ``` LOG(WARNING) << "Framework " << oldInfo.id() << " which had a principal " << " '" << oldPrincipal.getOrElse("<NONE>") << "'" << " tried to (re)subscribe with a new principal " << " '" << newPrincipal.getOrElse("<NONE>") << "'"; ``` src/master/validation.cpp Lines 595 (patched) <https://reviews.apache.org/r/70666/#comment302053> newline Also, not yours, but we generally don't add the period at the end. - Benjamin Mahler On May 17, 2019, 3:05 p.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70666/ > ----------------------------------------------------------- > > (Updated May 17, 2019, 3:05 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-7258 > https://issues.apache.org/jira/browse/MESOS-7258 > > > Repository: mesos > > > Description > ------- > > This patch moves the code validating a new `FrameworkInfo` against > the current one into a separate function. > > > Diffs > ----- > > src/master/master.cpp c72b92656dddca63ee89abf676da66bd76f58a6d > src/master/validation.hpp 95638a17052ece6c957aa76e4cead8d7bfe82024 > src/master/validation.cpp 9fb0850987ce385d345302cac9721adead7181b8 > > > Diff: https://reviews.apache.org/r/70666/diff/1/ > > > Testing > ------- > > > Thanks, > > Andrei Sekretenko > >