> On Nov. 21, 2017, 6:27 p.m., Alexander Rukletsov wrote: > > src/master/allocator/mesos/hierarchical.cpp > > Lines 430-437 (original), 431-438 (patched) > > <https://reviews.apache.org/r/63831/diff/1/?file=1894169#file1894169line431> > > > > Do we need a lambda here? > > Jiang Yan Xu wrote: > I kept the original use of lambdas. > > I think the benefit of labmdas here is more about readability: compared > to > > ``` > set<string> removedRoles = oldRoles; > foreach (const string& role, newRoles) { > result.erase(role); > } > ``` > > The constness and construction of the variable is clearly isolated into a > small block in a long method with many similar variables/code blocks. > > However as pointed out by the TODO, we should probably just implement a > set difference operator so all these become one-liners. I'll do it later.
Sounds like a wrong tool for the job and hence confusing. I would have used an explicit scope then. But maybe this is just me. I'll ask MPark for the reasoning here. Since it has been here before, it's fine to leave it for now. Regading introducing the set operator, FYI: https://reviews.apache.org/r/57110/#comment239761 - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63831/#review191613 ----------------------------------------------------------- On Nov. 28, 2017, 12:45 a.m., Jiang Yan Xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63831/ > ----------------------------------------------------------- > > (Updated Nov. 28, 2017, 12:45 a.m.) > > > Review request for mesos and Alexander Rukletsov. > > > Bugs: MESOS-8223 > https://issues.apache.org/jira/browse/MESOS-8223 > > > Repository: mesos > > > Description > ------- > > In `updateFramework`, we currently treat frameworks suppressing a role the > same way as frameworks moving off a role and this could lead to the framework > being removed from the framework sorter, which is unexpected. We should only > remove the framework from a framework sorter when it's moving away from the > corresponding role. > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.cpp > 5ce9ceaa3a5f84a1e076d45448863c418531cc2b > src/tests/scheduler_tests.cpp 45fc9c0cfccdb22c2e3e8d5de30c04575814a0e9 > > > Diff: https://reviews.apache.org/r/63831/diff/2/ > > > Testing > ------- > > make check. > > Added a test. > > > Thanks, > > Jiang Yan Xu > >
