----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54062/#review156878 -----------------------------------------------------------
include/mesos/roles.hpp (line 87) <https://reviews.apache.org/r/54062/#comment227206> I think it may not good to do the validatioin here as the validation has some logic related to framework capability and not pure role validation, you are actually validating frameworkinfo here. What about moving this to master/validation.cpp by adding a new namespace of `framework` for validation? src/common/roles.cpp (lines 110 - 130) <https://reviews.apache.org/r/54062/#comment227207> Can we simplify the logic as your diagram above: ``` if (frameworkInfo.roles_size() > 0 && frameworkInfo.has_role()) { } if (frameworkHasCapability( frameworkInfo, FrameworkInfo::Capability::MULTI_ROLE)) { if (frameworkInfo.roles_size() == 0 && frameworkInfo.has_role()) { } } else { if (frameworkInfo.roles_size() > 0 && !frameworkInfo.has_role()) { } } ``` src/common/roles.cpp (lines 116 - 117) <https://reviews.apache.org/r/54062/#comment227213> We prefer putting the blank space at the beginning. ``` return Error("FrameworkInfo.role should NOT be set when MULTI_ROLE" " framework capability is provided."); ``` Ditto for other messages. src/common/roles.cpp (line 121) <https://reviews.apache.org/r/54062/#comment227210> We do not want to use `.roles_size()` as boolean, but should be `if (frameworkInfo.roles_size() > 0) {`. src/common/roles.cpp (line 124) <https://reviews.apache.org/r/54062/#comment227214> s/must/can src/common/roles.cpp (line 138) <https://reviews.apache.org/r/54062/#comment227208> I think that you do not need this as there is no need to persist roles in this validation. src/common/roles.cpp (line 150) <https://reviews.apache.org/r/54062/#comment227209> kill this as well src/master/master.cpp (lines 2330 - 2335) <https://reviews.apache.org/r/54062/#comment227216> Can you please also add a new test case to test the register failed with mutiple roles in roles_tests.cpp like https://github.com/apache/mesos/blob/master/src/tests/master_tests.cpp#L4892 src/tests/role_tests.cpp (lines 666 - 717) <https://reviews.apache.org/r/54062/#comment227217> You may want to put this to master_validation_test.cpp if above comments addressed. src/tests/role_tests.cpp (line 706) <https://reviews.apache.org/r/54062/#comment227211> period to the end. src/tests/role_tests.cpp (line 710) <https://reviews.apache.org/r/54062/#comment227212> period to the end. - Guangya Liu On Nov. 24, 2016, 3:30 p.m., Jay Guo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54062/ > ----------------------------------------------------------- > > (Updated Nov. 24, 2016, 3:30 p.m.) > > > Review request for mesos and Guangya Liu. > > > Bugs: MESOS-6629 > https://issues.apache.org/jira/browse/MESOS-6629 > > > Repository: mesos > > > Description > ------- > > For backwards compatibility, we keep role in FrameworkInfo and do > necessary validation, which complies with following matrix: > > -- MULTI_ROLE is NOT set - > |-------|---------| > |Roles |No Roles | > |-------|-------|---------| > |Role | Error | None | > |-------|-------|---------| > |No Role| Error | None | > |-------|-------|---------| > > --- MULTI_ROLE is set ---- > |-------|---------| > |Roles |No Roles | > |-------|-------|---------| > |Role | Error | Error | > |-------|-------|---------| > |No Role| None | None | > |-------|-------|---------| > > > Diffs > ----- > > include/mesos/roles.hpp 1e6954447b821c2cf0dfa11373480161a0da5ae6 > src/common/roles.cpp 31774a9b8f99f5efeed35b1c3e3486e05ca00f6a > src/master/master.cpp e03a2e8025943825a2902102c43dc0eb66bacb6a > src/tests/role_tests.cpp 47fe0f06fb7728694445a62ccfe818fd9bf37c2c > > Diff: https://reviews.apache.org/r/54062/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jay Guo > >
