> On Feb. 10, 2017, 2:39 a.m., Benjamin Mahler wrote: > > src/master/validation.cpp, lines 1623-1630 > > <https://reviews.apache.org/r/55462/diff/3/?file=1627596#file1627596line1623> > > > > Just to clarify, this is an invariant in that if this fails it is a > > programming error in the master, right? It seems to me we have two > > invariants here: > > > > (1) Coming from framework: framework info is set and resources are > > allocated (master enforces this when applying operations). > > > > (2) Coming from operator: framework info is not set and resources are > > unallocated (master doesn't enforce this). > > > > We should clarify this. Also, `!resource.allocation_info().has_role()` > > is sufficient here if you want to be more succinct. > > Benjamin Bannier wrote: > I am not sure these cases are invariants *for this function*, and it > seems us ensuring correct behavior with tests rather then fail hard here > would decrease coupling between validation logic and apply operations and be > just as good. > > The case (1) we do explicitly handle here seems interesting since it > points to errors in framework code (e.g., incorrect handling of offered > resources). Case (2) seems less likely as it would mean that the operator > would set more information than needed or used by the master. In any case, > every caller of the validation function would need to normalize the resulting > operation in order to deal with the different possible inputs (e.g., (1) or > (2) here). > > Does that make sense?
Yeah this was just to clarify we had the same understanding, and it sounds like we do. I'm also in favor of not failing hard here. The other thing I would mention to be sure we both understand the same is that the operations won't apply later if case (1) or case (2) don't hold, even if we don't validate against these cases explicitly here. I wonder if we should also be validating case (2) since we're validating case (1), since validating it here allows us to provide a more informative message. E.g. "A reserve operation was attempted on allocated resources, but operators can only reserve available resources". Feel free to add it in this patch or another patch. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55462/#review165056 ----------------------------------------------------------- On Feb. 10, 2017, 3:16 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55462/ > ----------------------------------------------------------- > > (Updated Feb. 10, 2017, 3:16 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 > 0c2649089d7fd29eb021ac75c71e6a74368577dc > > Diff: https://reviews.apache.org/r/55462/diff/ > > > Testing > ------- > > N/A yet. > > > Thanks, > > Benjamin Bannier > >
