> On Jan. 13, 2017, 4:15 p.m., Jay Guo wrote: > > src/master/master.cpp, lines 3944-3950 > > <https://reviews.apache.org/r/55461/diff/1/?file=1603853#file1603853line3944> > > > > I think we explicitly disallow empyt role field? > > https://github.com/apache/mesos/blob/master/src/common/roles.cpp#L67 > > Also, should the case `framework->info.has_role() == false && > > framework->info.role() != "*"` be invalid and caught upon subscription? > > Benjamin Bannier wrote: > Multi-role frameworks would use the `roles` field, in which case the > `role` field would not be set. We need to distinguish here whether a > framework uses `role` or `roles`. The check can be simplified to > `framework->info.role() != "*"` though, independently of what we assume about > this field upstream. I made that change. > > Does that address your comment? > > Jay Guo wrote: > then why not using framework capability here? note that > https://reviews.apache.org/r/55021/ is landed which you may want to use. > > Benjamin Bannier wrote: > While this makes the code somewhat more explicit, I am not sure this > would make the code more readable. If the framework was not multi-role, we'd > still need to check `role != '*'` explicitly since we map `* -> {}` and `A -> > {A}` when invoking `validate`. This code could e.g., look like > > const hashset<string> frameworkRoles = > framework->capabilities.multiRole > ? hashset<string>( > {framework->info.roles().begin(), > framework->info.roles().end()}) > : framework->info.role() ! = > "*" ? hashset<string>({framework->info.role()}) > : hashset<string>(); > > Let me know if you feel that that is more readable and I'll update the > code.
Yeah I agree that yours looks more concise. Let's hear from other folks @gyliu @bmahler. - Jay ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55461/#review161488 ----------------------------------------------------------- On Jan. 16, 2017, 8:27 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55461/ > ----------------------------------------------------------- > > (Updated Jan. 16, 2017, 8:27 p.m.) > > > Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu. > > > Bugs: MESOS-6730 > https://issues.apache.org/jira/browse/MESOS-6730 > > > Repository: mesos > > > Description > ------- > > This updates the resource reservation validation for frameworks which > can have multiple roles. During a deprecation period 'FrameworkInfo' > will have fields for both 'role' and 'roles', however the validation > function works with just an optional set of roles. Here an empty set > captures the previous semantics of either having an empty 'role' field > or 'role' set as '*'. This forces the callers to properly construct a > set of framework roles from the available information. An optional set > is used in order to accommodate callers which have no information > about the framework's roles, and ultimately disables validation taking > that information into account. > > > Diffs > ----- > > src/master/master.cpp b863ff6e93931c3d1ee056248084c7f44caf2fd9 > src/master/validation.hpp 57e81779ff7444904c2ad7bad33aaf9167b98d05 > src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f > src/tests/master_validation_tests.cpp > e5d55e03648cb218d42adc594d6fa7d40ea9bcbb > > Diff: https://reviews.apache.org/r/55461/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Bannier > >
