> On Jan. 24, 2017, 3:32 p.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(...); > > } > > ``` > > Benjamin Bannier wrote: > 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 :/
I'm fine dropping this. I don't believe that we actually run into the lambda in header issue here since we don't actually pass the lambda to another function, but I think the right thing to do here is consider introducing a set of helpers for `FrameworkInfo`, where `getRoles` could be one of the candidates. - Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55271/#review162878 ----------------------------------------------------------- On Jan. 24, 2017, 4:41 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55271/ > ----------------------------------------------------------- > > (Updated Jan. 24, 2017, 4:41 p.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 > >
