mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrei Sekretenko <asekrete...@apache.org>
Subject Re: Review Request 72956: Added validation that offer constraints are set only for existing roles.
Date Tue, 27 Oct 2020 19:36:40 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message