> On Dec. 1, 2016, 10:36 p.m., Benjamin Mahler wrote: > > src/master/validation.cpp, lines 245-290 > > <https://reviews.apache.org/r/54062/diff/6/?file=1572831#file1572831line245> > > > > We could pull out a `multiRole` boolean here and perform validation > > based on it, this might make the MULTI_ROLE vs non-MULTI_ROLE distinction a > > bit clearer. I also noticed that the first check (for both being set) isn't > > necessary? > > > > Here's a suggestion for this logic: > > > > ``` > > bool multiRole = frameworkHasCapability( > > frameworkInfo, > > FrameworkInfo::Capability::MULTI_ROLE; > > > > // Ensure that the right fields are used. > > if (multiRole) { > > if (frameworkInfo.has_role()) { > > return Error("'FrameworkInfo.role' must not be set when the > > framework is MULTI_ROLE capable"); > > } > > } else { > > if (frameworkInfo.roles_size() > 0) { > > return Error("'FrameworkInfo.roles' must not be set when the > > framework is not MULTI_ROLE capable"); > > } > > } > > > > // Check for duplicate entries. > > // > > // TODO(bmahler): Use a generic duplicate check function. > > if (multiRole) { > > const hashset<string> duplicateRoles = [&]() { > > hashset<string> roles; > > hashset<string> duplicates; > > > > foreach (const string& role, frameworkInfo.roles()) { > > if (roles.contains(role)) { > > duplicates.insert(role); > > } > > roles.insert(role); > > } > > > > return duplicateRoles; > > }(); > > > > if (!duplicateRoles.empty()) { > > return Error("'FrameworkInfo.roles' contains duplicate items: " + > > stringify(duplicateRoles)); > > } > > } > > > > > > // Validate the role(s). > > if (multiRole) { > > foreach (const string& role, frameworkInfo.roles()) { > > Option<Error> error = validate(role); > > if (error) { > > return Error("'FrameworkInfo.roles' contains invalid role(s)" > > ": " + error->message); > > } > > } > > } else { > > Option<Error> error = validate(frameworkInfo.role()); > > if (error) { > > return Error("'FrameworkInfo.role' is not a valid role" > > ": " + error->message); > > } > > } > > > > return None(); > > ``` > > Qian Zhang wrote: > I think one case is missed here: `MULTI_ROLE` is set but > `FrameworkInfo.roles` is empty, in this case, I think we should return an > error since framework must have at least one role in order to be offered > resources.
This is a valid case and framework with this setting will not get any resource allocated. - Jay ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54062/#review157651 ----------------------------------------------------------- On Dec. 2, 2016, 8:44 a.m., Jay Guo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54062/ > ----------------------------------------------------------- > > (Updated Dec. 2, 2016, 8:44 a.m.) > > > Review request for mesos, Benjamin Mahler, Guangya Liu, and Qiang Zhang. > > > Bugs: MESOS-6629 > https://issues.apache.org/jira/browse/MESOS-6629 > > > Repository: mesos > > > Description > ------- > > Roles should not contain duplicate entries. Also, we need to validate > that 'role', 'roles' and MULTI_ROLE capability are set properly. They > are checked against 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 > ----- > > src/master/validation.hpp b8389460f34b3531f2b6ff93f18f496c01e1a079 > src/master/validation.cpp 42d9b4a8784c2a161b74d7b46619cc22272e14e3 > > Diff: https://reviews.apache.org/r/54062/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jay Guo > >
