----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55462/#review165404 -----------------------------------------------------------
src/master/validation.cpp (lines 1617 - 1629) <https://reviews.apache.org/r/55462/#comment237238> We should be able to just remove these per the discussion from the last review. src/master/validation.cpp (line 1668) <https://reviews.apache.org/r/55462/#comment237242> s/+s/+ s/ src/master/validation.cpp (lines 1668 - 1670) <https://reviews.apache.org/r/55462/#comment237243> Hm.. the ", but" line seems a bit odd, how about: ``` return Error( "A reserved operation was attempted on unallocated resource" " '" +stringify(resource) + "', but frameworks can only" " perform reservations on allocated resources"); ``` src/master/validation.cpp (lines 1675 - 1677) <https://reviews.apache.org/r/55462/#comment237244> Is it possible to get the quotes on the same line? E.g. ``` return Error( "A reserve operation was attempted for a resource with role" " '" + resource.role() + "', but the resource was allocated" " to role '" + resource.allocation_info().role() + "'"); ``` src/master/validation.cpp (lines 1682 - 1686) <https://reviews.apache.org/r/55462/#comment237237> Is it possible to put the quotes on the same line? ``` return Error( "A reserve operation was attempted for a resource allocated to role" " '" + resource.role() + "', but the framework only has roles" " '" + stringify(frameworkRoles.get()) + "'"); ``` src/master/validation.cpp (line 1694) <https://reviews.apache.org/r/55462/#comment237245> s/"' "/" '"/ src/master/validation.cpp (lines 1698 - 1699) <https://reviews.apache.org/r/55462/#comment237239> For a bit more clarity: "set because operators can only reserve the available resources" src/master/validation.cpp (lines 1702 - 1703) <https://reviews.apache.org/r/55462/#comment237246> Hm.. what does "out-of-band" allocation info mean? How about something like the following, which makes it clear what the operator API allows: ``` A reserve operation was attempted with an allocated resource but the operator API only allows reservations to be made to the unallocated resources ``` src/tests/master_validation_tests.cpp (lines 424 - 425) <https://reviews.apache.org/r/55462/#comment237247> How about having this block right next to the reserve block since these two seem related, seems a bit clearer? src/tests/master_validation_tests.cpp (lines 471 - 490) <https://reviews.apache.org/r/55462/#comment237248> We could generalize this test to be: a framework (that has role `"*"` in its roles, e.g. `{role1, role2, *}`) cannot make a reservation to `*`. As it stands we seem to be missing the case where star is present but there are multiple roles. src/tests/master_validation_tests.cpp (lines 493 - 513) <https://reviews.apache.org/r/55462/#comment237250> This could be generalized to be making a reservation to a role that is not within its roles (which may be already caught by the mismatched allocation role since we expect allocation role == reserve role)? - Benjamin Mahler On Feb. 13, 2017, 3:38 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55462/ > ----------------------------------------------------------- > > (Updated Feb. 13, 2017, 3:38 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 change introduces validation of the 'AllocationInfo' of resources > used in reservations. > > > Diffs > ----- > > src/master/validation.cpp 2bfca95713319a195e43e990cbc2dc5570b89c4e > src/tests/master_validation_tests.cpp > fd1f4a6cf0661351e265e50da1bd6ea04ed13d26 > > Diff: https://reviews.apache.org/r/55462/diff/ > > > Testing > ------- > > N/A yet. > > > Thanks, > > Benjamin Bannier > >
