mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Rukletsov <ruklet...@gmail.com>
Subject Re: Review Request 63831: Fixed a bug that removed the suppressed framework from sorter.
Date Fri, 01 Dec 2017 14:45:28 GMT


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


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