mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 55271: Disallow multi-role frameworks to change their roles.
Date Wed, 25 Jan 2017 00:41:37 GMT


> On Jan. 25, 2017, 12:32 a.m., Michael Park wrote:
> > src/master/master.hpp, lines 2490-2504
> > <https://reviews.apache.org/r/55271/diff/15/?file=1612323#file1612323line2490>
> >
> >     What do you think about pulling the roles retrieval out?
> >     
> >     ```cpp
> >     auto getRoles = [](const FrameworkInfo& info) -> std::set<std::string>
{
> >       if (protobuf::frameworkHasCapability(
> >               info, FrameworkInfo::Capability::MULTI_ROLE)) {
> >         return {info.roles().begin(), info.roles().end())};
> >         
> >       } else {
> >         return {info.role()};
> >       }
> >     };
> >     
> >     if (getRoles(info) != getRoles(source)) {
> >       return Error(...);
> >     }
> >     ```

I like this pattern. It might not be much shorter, but only contains the logic once, and we
could use more "normal" control flow.

I would prefer to not use a lambda here though as certain compilers might incorrectly cause
ODR violations for some code involving lambdas in functions defined in headers (not sure it
is actually happening here, and we don't seem to follow a dedicated pattern to avoid this).
As ODR might invoke UB I'd prefer to just copy the code :/


- Benjamin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55271/#review162878
-----------------------------------------------------------


On Jan. 25, 2017, 1:41 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> -----------------------------------------------------------
> 
> (Updated Jan. 25, 2017, 1:41 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, Guangya Liu, and Michael Park.
> 
> 
> Bugs: MESOS-6631
>     https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 4bd01fd21820b7cdb6c2a23436d50e00fbbd5849 
>   src/tests/master_validation_tests.cpp ce10ea4502d0a13faca28d288dd16cd5cb864f6e 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> -------
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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