> On Feb. 11, 2017, 12:01 a.m., Benjamin Mahler wrote: > > src/master/validation.cpp, lines 1612-1629 > > <https://reviews.apache.org/r/55462/diff/4/?file=1629533#file1629533line1612> > > > > Looks like this needs a rebase against master? This block was removed > > and the roles are no longer an option. > > Benjamin Bannier wrote: > No, this patch was created against a recent `HEAD`. The modified patch > you committed as part of r/55461 introduced a loop-local `set<string> > frameworkRoles` inside the loop over resources. That is nice as it avoids > keeping two optional, related values in scope (`frameworkInfo` and > `frameworkRoles`). This comes at the cost of recalculating this framework > invariant for each loop iteration; I here pulled it out of the loop to not > recalculate it over and over again. Do you believe that's a bad idea?
Ok that makes sense. And what about the validation that is in this block? Based on the discussion from the last review these were removed, are you suggesting to add them back in? - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55462/#review165204 ----------------------------------------------------------- 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 > >
