> On Oct. 15, 2020, 11:22 p.m., Benjamin Mahler wrote: > > src/master/master.cpp > > Line 2622 (original), 2622-2624 (patched) > > <https://reviews.apache.org/r/72956/diff/2/?file=2240748#file2240748line2622> > > > > ``` > > Make sure the offer constraints don't use roles other than the > > framework's roles. > > ``` > > > > That seems sufficient for the reader? I'm not sure they need to be > > explained here that you could construct a "valid" OfferConstraintsFilter > > with roles outside of the framework's roles
Fully agree that my previous version probably doesn't add much value. On the other hand, the shorter version still has to be kept it in sync with the code of `validateOfferConstraintsRoles()`. Removed this comment altogether, hopefully the function name is descriptive enough. > On Oct. 15, 2020, 11:22 p.m., Benjamin Mahler wrote: > > src/master/validation.hpp > > Lines 137-139 (patched) > > <https://reviews.apache.org/r/72956/diff/2/?file=2240749#file2240749line137> > > > > Similar to the last review, just want to comment on how these are unit > > testable. In this case, I'm not sure how we could avoid the integration > > test though since it's not a sub-case of validation where we can just > > ensure in the integration test that overall validation is called. > > > > Still, might want to just add the simple unit test for this? I'll probably send a follow-up patch with unit tests, for this, and also for the suppressed roles validation. > On Oct. 15, 2020, 11:22 p.m., Benjamin Mahler wrote: > > src/master/validation.cpp > > Lines 706 (patched) > > <https://reviews.apache.org/r/72956/diff/2/?file=2240750#file2240750line706> > > > > could just use `foreachkey (` > > > > ``` > > foreachkey (const string& role, offerConstraints.role_constraints()) { > > > > } > > ``` Unfortunatley, there is no straightforward way to use a protobuf map with `foreachkey()`. This macro employs `std::get<>`, which is not overloaded for protobuf's `MapPair`. - Andrei ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72956/#review222061 ----------------------------------------------------------- On Oct. 14, 2020, 8:18 p.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72956/ > ----------------------------------------------------------- > > (Updated Oct. 14, 2020, 8:18 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-10176 > https://issues.apache.org/jira/browse/MESOS-10176 > > > Repository: mesos > > > Description > ------- > > This patch makes SUBSCRIBE/UPDATE_FRAMEWORK calls validate that > the framework does not specify offer constraints for roles to which > it is not going to be subscribed. > > > Diffs > ----- > > src/master/master.cpp d6d3ea7e27539a65b601783eca2ac4e3b688f188 > src/master/validation.hpp 7fe8f087b322f8e77218f4dd9f9f75707afa3208 > src/master/validation.cpp 5b1bcb5207aac0553bf9c1a0d89525c835393ccb > src/tests/master/update_framework_tests.cpp > 3f86573e8dfeea63fe99064f2137c61d901f4fc7 > > > Diff: https://reviews.apache.org/r/72956/diff/3/ > > > Testing > ------- > > `make check` > `src/mesos-tests > --gtest_filter='*UpdateFrameworkTest.OfferConstraintsForUnsubscribedRole*' > --gtest_break_on_failure --gtest_repeat=1000` > > > Thanks, > > Andrei Sekretenko > >
