----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56939/#review166462 -----------------------------------------------------------
High level thoughts: (1) Can you split out the validation for task / executor / volume / reservation? That will make it easier to review and commit them piece by piece. (2) Looks like there are no tests for volumes and reservations? src/master/validation.cpp (line 588) <https://reviews.apache.org/r/56939/#comment238415> How about 'validateSingleAllocationRole'? src/master/validation.cpp (lines 602 - 603) <https://reviews.apache.org/r/56939/#comment238417> These error messages compose a bit oddly: ``` Executor mixes allocation roles: Resources mixes allocations roles: role1 and role2 ``` How about: ``` Invalid executor resources: The resources have multiple allocation roles ('role1' and 'role2') but only one allocation role is allowed ``` Here `"Invalid executor resources:"` comes from the caller and `"The resources have multiple allocation roles ('role1' and 'role2') but only one allocation role is allowed"` comes from the helper function. src/master/validation.cpp (line 1658) <https://reviews.apache.org/r/56939/#comment238414> Why couldn't you use `resource::validateAllocationRole(reserve.resources())` here? - Benjamin Mahler On Feb. 22, 2017, 5:43 p.m., Jay Guo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56939/ > ----------------------------------------------------------- > > (Updated Feb. 22, 2017, 5:43 p.m.) > > > Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and > Michael Park. > > > Bugs: MESOS-6636 > https://issues.apache.org/jira/browse/MESOS-6636 > > > Repository: mesos > > > Description > ------- > > With support for multi-role frameworks, we need to make sure that > individual tasks and executors cannot mix roles. Likewise, we do > not want to allow a scheduler to make a reservation or create a > volume based on resources with different allocated roles. > > > Diffs > ----- > > src/master/validation.hpp f03b3280704083be1ca074ca07c69edbb49dae19 > src/master/validation.cpp b8c6b49ca5ca1f15e58a2967adc335436a35071f > src/tests/master_validation_tests.cpp > 53771f6b5492009fe75cbbfc03a2b542856c1070 > > Diff: https://reviews.apache.org/r/56939/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jay Guo > >
