----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55271/#review162535 -----------------------------------------------------------
src/master/master.hpp (lines 2475 - 2516) <https://reviews.apache.org/r/55271/#comment233843> We set the `validationError` to `None` just above, so this should always be `true`. It looks to me like this used to be a loop? It looks like this could be: ```cpp if (protobuf::frameworkHasCapability( info, FrameworkInfo::Capability::MULTI_ROLE) || protobuf::frameworkHasCapability( source, FrameworkInfo::Capability::MULTI_ROLE)) { // ... if (oldRoles != newRoles) { return Error(...); } } ``` src/master/master.hpp (line 2481) <https://reviews.apache.org/r/55271/#comment233841> We set the `validationError` to `None` just above, so this should always be `true`. It looks to me like this used to be a loop? It looks like this could be: ```cpp if (protobuf::frameworkHasCapability( info, FrameworkInfo::Capability::MULTI_ROLE) || protobuf::frameworkHasCapability( source, FrameworkInfo::Capability::MULTI_ROLE)) { // ... if (oldRoles != newRoles) { return Error(...); } } ``` src/tests/master_validation_tests.cpp (lines 2718 - 2721) <https://reviews.apache.org/r/55271/#comment233846> This is quite brittle, not only because the error message could change, but because we print out a `hashset`, the ordering is not deterministic... I might actually suggest using `std::set` so that we get alphabetical ordering of the role names which I think would be easier to read/compare in an error message. src/tests/master_validation_tests.cpp (lines 2723 - 2724) <https://reviews.apache.org/r/55271/#comment233845> Shift indentation to the left, here and below. - Michael Park On Jan. 20, 2017, 9:53 a.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55271/ > ----------------------------------------------------------- > > (Updated Jan. 20, 2017, 9:53 a.m.) > > > Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu. > > > Bugs: MESOS-6631 > https://issues.apache.org/jira/browse/MESOS-6631 > > > Repository: mesos > > > Description > ------- > > We currently do not allow `MULTI_ROLE` frameworks to change their > roles. This restriction will be lifted later. > > > Diffs > ----- > > src/master/master.hpp 8e8a9037af11cf95961b6498540a0fd486ed091b > src/tests/master_validation_tests.cpp > a63178139a5283d6a3fcbe60c271dab1914e5da9 > > Diff: https://reviews.apache.org/r/55271/diff/ > > > Testing > ------- > > Tested on various Linux configurations in internal CI. > > > Thanks, > > Benjamin Bannier > >
