----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55461/#review164410 -----------------------------------------------------------
src/master/master.cpp (lines 3942 - 3947) <https://reviews.apache.org/r/55461/#comment236112> 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. src/master/validation.hpp (lines 223 - 227) <https://reviews.apache.org/r/55461/#comment236110> Hm.. any reason this shouldn't follow the pattern used in the create validation right below where we take framework info? That seems to make it a bit clearer that the optional framework means it's none when done by operator. E.g. ``` // Validates the RESERVE operation. Option<Error> validate( const Offer::Operation::Reserve& reserve, const Option<std::string>& principal, const Option<FrameworkInfo>& frameworkInfo = None()); ``` src/master/validation.cpp (lines 1500 - 1503) <https://reviews.apache.org/r/55461/#comment236113> 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? src/master/validation.cpp (lines 1509 - 1513) <https://reviews.apache.org/r/55461/#comment236115> 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? - Benjamin Mahler On Jan. 16, 2017, 12: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, 12: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 > >
