----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54062/#review156877 -----------------------------------------------------------
src/common/roles.cpp (lines 112 - 118) <https://reviews.apache.org/r/54062/#comment227218> What if the `MULTI_ROLE` capability is provided, but `frameworkInfo.roles` is empty? So in this case framework will be considered to subscribe to `"*"` role? And based on the description in MESOS-6629, only one of FrameworkInfo.role and FrameworkInfo.roles **MUST** be set at a time. But it seems in your code here, both FrameworkInfo.role and FrameworkInfo.roles can be unset at a time and can pass the validation in this method, however I think that should be an invalid case. src/common/roles.cpp (line 119) <https://reviews.apache.org/r/54062/#comment227203> Kill this line. - Qian Zhang On Nov. 24, 2016, 11: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, 11: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 > >
