----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57111/#review167882 -----------------------------------------------------------
src/master/allocator/mesos/hierarchical.hpp Lines 426-429 (original), 426-429 (patched) <https://reviews.apache.org/r/57111/#comment239845> Ditto suggestion from previous review for updating this comment, and also removing the notion of "active" here. src/master/allocator/mesos/hierarchical.cpp Lines 429-432 (original), 394-400 (patched) <https://reviews.apache.org/r/57111/#comment239864> Ditto from last review, could we simplify with a - operator in stout/set.hpp? ``` const set<string> removedRoles = oldRoles - newRoles; ``` src/master/allocator/mesos/hierarchical.cpp Lines 417-423 (patched) <https://reviews.apache.org/r/57111/#comment239865> Ditto from last review, could we simplify with a - operator in stout/set.hpp? ``` const set<string> addedRoles = newRoles - oldRoles; ``` src/master/allocator/mesos/hierarchical.cpp Lines 426-429 (patched) <https://reviews.apache.org/r/57111/#comment239883> Hm.. this was a bit tough to grasp for me, how about something like: ``` // Add the framework to the role. It's possible that we're already // tracking this role for the framework because a framework can // unsubscribe from a role while it still has resources allocated // to the role. ``` src/master/allocator/mesos/hierarchical.cpp Line 458 (original), 463 (patched) <https://reviews.apache.org/r/57111/#comment239886> Can you commit this separately? src/master/allocator/mesos/hierarchical.cpp Lines 470-473 (patched) <https://reviews.apache.org/r/57111/#comment239895> Maybe something a bit more general, since this isn't the only case where this is possible and that might confuse the reader. It's also possible when there is no partition involved, when the master is re-registering an agent. E.g. ``` // The framework has resources allocated to this role but it may // or may not be subscribed to the role. Either way, we need to // track the framework under the role. ``` src/master/allocator/mesos/hierarchical.cpp Lines 1065-1071 (patched) <https://reviews.apache.org/r/57111/#comment239912> Hm.. maybe we can use "start tracking" and "stop tracking" in these comments since "adding" "removing" from the role seems a little less clear to me. E.g. ``` Stop tracking the framework under this role if it's no longer subscribed and no longer has resources allocated to the role. ``` src/master/allocator/mesos/hierarchical.cpp Lines 1066 (patched) <https://reviews.apache.org/r/57111/#comment239896> s/no/no longer any/ src/master/allocator/mesos/hierarchical.cpp Lines 2186-2187 (patched) <https://reviews.apache.org/r/57111/#comment239855> Subscribe *or have resources allocated to this role*, right? src/master/allocator/mesos/hierarchical.cpp Lines 2191 (patched) <https://reviews.apache.org/r/57111/#comment239861> Do you want a CHECK in front of this insert, since you seem to be check guarding every other insert in this function? src/master/allocator/mesos/hierarchical.cpp Lines 2218 (patched) <https://reviews.apache.org/r/57111/#comment239862> This condition isn't quite right anymore, maybe something like: ``` If no more frameworks are subscribed to this role or have resources allocated to this role, ... ``` src/master/allocator/mesos/hierarchical.cpp Lines 2228 (patched) <https://reviews.apache.org/r/57111/#comment239849> Did you want to use an `.at()` here? - Benjamin Mahler On Feb. 27, 2017, 10:20 p.m., Michael Park wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57111/ > ----------------------------------------------------------- > > (Updated Feb. 27, 2017, 10:20 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-6627 > https://issues.apache.org/jira/browse/MESOS-6627 > > > Repository: mesos > > > Description > ------- > > See summary. > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.hpp > d33306745a7287b750cb4a5242c7527369d58d65 > src/master/allocator/mesos/hierarchical.cpp > eeb44fe89d4bfd26900b11833c1182157e5c7e5c > > > Diff: https://reviews.apache.org/r/57111/diff/1/ > > > Testing > ------- > > > Thanks, > > Michael Park > >
