> On Oct. 14, 2020, 12:54 a.m., Benjamin Mahler wrote: > > src/master/master.hpp > > Lines 1061-1062 (original), 1061-1062 (patched) > > <https://reviews.apache.org/r/72955/diff/1/?file=2240632#file2240632line1061> > > > > Maybe just say "based on master flags and the framework not being > > completed"? > > > > Also, this makes it sound like it only validates things against the > > master flags and the framework being non-completed, but it also validates > > somewhat that the FrameworkInfo is valid on its own, like the failover > > timeout. Should the failover timeout validation be moved to the existing > > stateless validation of FrameworkInfo?
Moving the failover timeout into stateless validation is a right thing, sent a patch for that. Nevertheless, the very first thing this function does is calling the stateless validation, and I have no intention to change this now. I've reworded this comment change to avoid giving impression that stateless validation is performed separately. > On Oct. 14, 2020, 12:54 a.m., Benjamin Mahler wrote: > > src/master/master.cpp > > Lines 2588-2598 (original) > > <https://reviews.apache.org/r/72955/diff/1/?file=2240633#file2240633line2589> > > > > Per above comment, did you mean to also move out the failover timeout > > validation, which looks to be a similarly stateless validation of the > > FrameworkInfo as this bit? > > Andrei Sekretenko wrote: > Thanks! I somehow missed that it is entirely stateless. Will send one > more small patch. Patch for moving failover timeout validation: https://reviews.apache.org/r/72964/ > On Oct. 14, 2020, 12:54 a.m., Benjamin Mahler wrote: > > src/master/master.cpp > > Lines 2629-2634 (patched) > > <https://reviews.apache.org/r/72955/diff/1/?file=2240633#file2240633line2641> > > > > Hm.. shouldn't this stateless FrameworkInfo validation go in the > > existing `validation::framework::valdiate(const FrameworkInfo&)` function > > in master/validation.hpp? I would say the this validates not `FrameworkInfo` but rather suppressed roles, which we pass separately (and do not get from resubscribing agents, only via scheduler API). Now this patch moves this validation into a separated function in `validation.hpp`; not sure if it makes things better or worse, though. - Andrei ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72955/#review222039 ----------------------------------------------------------- On Oct. 14, 2020, 8:16 p.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72955/ > ----------------------------------------------------------- > > (Updated Oct. 14, 2020, 8:16 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/3/ > > > Testing > ------- > > make check > > > Thanks, > > Andrei Sekretenko > >
