> On Feb. 6, 2017, 10:28 p.m., Benjamin Mahler wrote: > > src/master/validation.cpp, lines 1500-1503 > > <https://reviews.apache.org/r/55461/diff/3/?file=1605766#file1605766line1500> > > > > Hm.. do you know which call sites need to be updated? It seems like we > > need to do the sweep now to ensure the call-sites are correct, no?
This was an in process-`TODO`, the call sites should be updated now. > On Feb. 6, 2017, 10:28 p.m., Benjamin Mahler wrote: > > src/master/master.cpp, lines 3942-3947 > > <https://reviews.apache.org/r/55461/diff/3/?file=1605764#file1605764line3942> > > > > This isn't quite correct since we need to check the capability to > > determine which field to examine. > > > > Per my suggestion below, can we pass the FrameworkInfo in rather than > > just the roles? > > > > Also note that there is a `getRoles` helper in protobuf utils to > > simplify this. We should add a `set<string> roles` field to the `Framework` > > struct in the master though. Updated to pass an `Option<FrameworkInfo>`. > On Feb. 6, 2017, 10:28 p.m., Benjamin Mahler wrote: > > src/master/validation.cpp, lines 1509-1513 > > <https://reviews.apache.org/r/55461/diff/3/?file=1605766#file1605766line1509> > > > > This condition isn't quite the `*` role, this would be a framework with > > no roles. The `*` role case is now the 1 element `*` set, but it seems to > > me this check should be looking at the resource.role() rather than the > > framework's roles, no? This definitely incorrectly handles single role frameworks in `{*}`. I updated the patch so we check for any both an empty roles set and `{*}` to handle both frameworks with no roles and frameworks in `{*}`. The cases where `*` is included in `roles` is supported, so framework roles `{*}` will remain a valid case even after `FrameworkInfo.role` is removed, right? - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55461/#review164410 ----------------------------------------------------------- On Feb. 8, 2017, 12:41 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55461/ > ----------------------------------------------------------- > > (Updated Feb. 8, 2017, 12:41 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 98c39b279e7b9830d02efc8ec6a4469afc15d62a > src/master/validation.hpp fc1eaeebd0643b897f4e14aabc76e7d94f0de263 > src/master/validation.cpp 37d171512ee54f260aabb6e1071739bcc3769fb0 > src/tests/master_validation_tests.cpp > 51185031cb67e64cd69ec6ce1c8f722a0c349970 > > Diff: https://reviews.apache.org/r/55461/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Bannier > >
