> On Oct. 15, 2020, 9:27 p.m., Benjamin Mahler wrote: > > src/master/master.cpp > > Lines 2615-2616 (patched) > > <https://reviews.apache.org/r/72955/diff/3/?file=2240753#file2240753line2627> > > > > It seems unfortunate that validatation of the suppressed roles is > > buried within creating the allocator framework options, which leads to it > > being modeled as a "failure to create the options" rather than a > > "validation error". It's less clear if the failure is due to invalidness or > > some kind of error on our part. > > > > The explicit approach makes it clear: > > > > ``` > > Option<Error> e = validateFramework(info); > > > > ... > > > > // Validate the other fields of the subscription: > > e = validateSuppressedRoles(info, suppressedRoles); > > ... > > e = validateOfferConstraints(info, offerConstraints); > > > > make allocator options w/ hard failure if not valid > > ``` > > > > Also, by containing all of the API validation within our validation > > headers we can unit test all of it, and for integration tests we only need > > to test that the functions are called (rather than testing all invalid > > cases via the integration tests). That was the idea when we started to > > contain it all within validation headers. > > > > Just the thought I had while reading this, not suggesting that you > > necessarily change this.
Left it as it is for now. Probably it will make sense to revisit this location if, for example, the stateless validation of `FrameworkInfo` becomes better separated from validation against Master state. Fully agree that the fact that **all** errors returned by `createAllocatorFrameworkOptions()` (and by `OfferConstraintsFilter::create()`, for that matter!) are **validation errors** is relatively non-straightforward. Note that this approach was chosen mainly because we in any case need an intermediate representation due to the need to construct RE2s; thus, all the locations in the code that can encounter invalid offer constraints are localized in the IR construction. In this case, having a validation code that would simply mirror construction (and will construct RE2s one more time) made no sense. I would say that this is not the case in many other data processing paths in Mesos, where validation doesn't really mirror any code that is using the validated data afterwards. In these cases, validation still needs to be kept in sync with what follows next, but we cannot realistically avoid this, as it is either expensive, or outright impossible to guard against an invalid input later on, or unreasonably difficult to propagate an error backwards. The offer constraints IR construction is not among these cases, though. > On Oct. 15, 2020, 9:27 p.m., Benjamin Mahler wrote: > > src/master/validation.cpp > > Lines 692-693 (patched) > > <https://reviews.apache.org/r/72955/diff/3/?file=2240755#file2240755line692> > > > > don't know if we need the quotes here, since we're not showing a string > > field: > > > > `Suppressed roles {"foo", "bar"} are not ...` We don't need them; fixed this. - Andrei ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72955/#review222060 ----------------------------------------------------------- On Oct. 27, 2020, 8:38 p.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72955/ > ----------------------------------------------------------- > > (Updated Oct. 27, 2020, 8:38 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-10176 > https://issues.apache.org/jira/browse/MESOS-10176 > > > Repository: mesos > > > Description > ------- > > This merges three near-identical pieces of scattered code in SUBSCRIBE > and UPDATE_FRAMEWORK execution paths in the Master that validate > and construct parts of `allocator::FrameworkOptions` (the set of > suppressed roles and the offer constraints filter) into a single > function. > > This is a prerequisite to adding validation of offer constraint roles. > > > Diffs > ----- > > src/master/master.hpp 83d8190702ff25b5e3e9607d8250aa331aaad028 > src/master/master.cpp d6d3ea7e27539a65b601783eca2ac4e3b688f188 > src/master/validation.hpp 7fe8f087b322f8e77218f4dd9f9f75707afa3208 > src/master/validation.cpp 5b1bcb5207aac0553bf9c1a0d89525c835393ccb > > > Diff: https://reviews.apache.org/r/72955/diff/4/ > > > Testing > ------- > > make check > > > Thanks, > > Andrei Sekretenko > >
