----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54524/#review158717 -----------------------------------------------------------
Ship it! This could benefit from a clearer commit summary and description: ``` Updated `Role::resources()` in master to handle multi-role frameworks. Currently, the `Role::resources()` method in the `master::Role` struct returns the total resources used and offered to a framework, which will be incorrect for multi-role frameworks. For now, since the support for multi-role frameworks is incomplete (in particular, the `Resource.allocation_info` is not yet populated), we continue to handle single-role frameworks in the old manner. Once the `Resource.allocation_info` is populated, we will remove the single-role special case logic in `Role::resources()`. ``` src/master/master.hpp (lines 2686 - 2689) <https://reviews.apache.org/r/54524/#comment229523> Comment sentences start with a capital letter. Maybe we should re-word this a bit to clarify: ``` // TODO(jay_guo): MULTI_ROLE is handled as a special case because // the `Resource.allocation_info.role` is not yet populated. Once // the support is complete, we do not need the `else` logic here. ``` src/master/master.hpp (line 2692) <https://reviews.apache.org/r/54524/#comment229520> Hm.. it seems this might be a bit clearer if we pass the role explicitly here rather than capturing it above: ``` resources += framework->totalUsedResources.filter(allocatedTo(role)); ``` You would need: ``` auto allocatedTo = [](const std::string& role) { return [role](const Resource& resource) { return resource.allocation_info().role() == role; }; }; ``` - Benjamin Mahler On Dec. 9, 2016, 4:57 p.m., Jay Guo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54524/ > ----------------------------------------------------------- > > (Updated Dec. 9, 2016, 4:57 p.m.) > > > Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu. > > > Bugs: MESOS-6685 > https://issues.apache.org/jira/browse/MESOS-6685 > > > Repository: mesos > > > Description > ------- > > Currently `resources()` method in `struct Role` of master.hpp returns > totalUsedResources and totalOfferedResources for a framework, which is > inaccurate for multi-role frameworks. A filter is added to capture the > actual resources for specific role in multi-role frameworks. > > > Diffs > ----- > > src/master/master.hpp d3c20d7cbb9137d3b05cae3d67f004d5e7475c8a > src/master/master.cpp 353e6ea802e197b4456c1647f78d9984a50f1c9d > > Diff: https://reviews.apache.org/r/54524/diff/ > > > Testing > ------- > > make check on Ubuntu 14.04 > > > Thanks, > > Jay Guo > >
