----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65939/#review200584 -----------------------------------------------------------
Hey Till, I have added some comments. Most are minor style sthings which you should feel free to ignore :-). I have two high-level comments (we already talked about the first one but thought of putting it in here anyways): 1. Backwards compatibility wrt to the Role proto for /roles endpoint and the likes. 2. I noticed that we have `master->roles.getRole()`, etc., scattered all over the code and wondering if we should put some of these methods in the Master class itself for better readability? include/mesos/mesos.proto Line 3407 (original), 3407-3409 (patched) <https://reviews.apache.org/r/65939/#comment281329> I wonder how this would affect backwards compatibility. Do we need to keep `resources` as is and add `allocated` and `total_allocated` in addition? If we do this, we'd probably need to figure out a way to handle old vs new frameworks. include/mesos/v1/master/master.proto Lines 458-459 (original), 458-459 (patched) <https://reviews.apache.org/r/65939/#comment281328> Please update the corresponding V0 master.proto as well. src/master/master.hpp Lines 139-140 (patched) <https://reviews.apache.org/r/65939/#comment281362> Now that we are keeping track of `allocatedResources` and `totalAllocatedResources`, we can replace this hashmap with a simple hashset. We don't seem to be using `Framework*` elsewhere. src/master/master.hpp Lines 173-174 (patched) <https://reviews.apache.org/r/65939/#comment281334> Can we change the third sentence ("An implicit ... active role.") to something like: An implicit role is not active - frameworks are connected to its child roles instead. src/master/master.hpp Lines 178 (patched) <https://reviews.apache.org/r/65939/#comment281337> I am not sure if I understand what `returned in ["a", "a/b"]` means. src/master/master.hpp Lines 181 (patched) <https://reviews.apache.org/r/65939/#comment281341> Would this search the framework in child roles as well? Can we expand to comment to explicitly mention it? Something like: ` Checks if the given framework is being tracked under the given role or its child roles` src/master/master.cpp Lines 495-501 (patched) <https://reviews.apache.org/r/65939/#comment281345> Just a minor comment. Would it improve readability if we were to switch the if condition with the CHECK condition? As in: ``` if (!roleMap.contains(path)) { CHECK(!parent->children.contains(path)); VLOG(1) ... ... } ``` src/master/master.cpp Lines 519 (patched) <https://reviews.apache.org/r/65939/#comment281347> s/stop going up the tree/not removing it Here and below. That way it applies to the top-level roles as well. src/master/master.cpp Lines 520 (patched) <https://reviews.apache.org/r/65939/#comment281346> s/break/return/ ? Here and below. src/master/master.cpp Lines 529 (patched) <https://reviews.apache.org/r/65939/#comment281350> What if we bring this line after VLOG line. Further, we need a `CHECK_NOTNULL(parent);` here? src/master/master.cpp Lines 531 (patched) <https://reviews.apache.org/r/65939/#comment281349> s/childless/leaf ? src/master/master.cpp Lines 572 (patched) <https://reviews.apache.org/r/65939/#comment281360> Wondering if we should use `getRole(role)->frameworks.contains(id)` for uniformity. src/master/master.cpp Lines 587 (patched) <https://reviews.apache.org/r/65939/#comment281361> Do we need a CHECK here, or is that too defensive? src/master/master.cpp Lines 619 (patched) <https://reviews.apache.org/r/65939/#comment281363> This VLOG seems to stand out. Do we need similar log messages in other functions (`addResources`, etc.)? - Kapil Arya On April 5, 2018, 12:15 p.m., Till Toenshoff wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65939/ > ----------------------------------------------------------- > > (Updated April 5, 2018, 12:15 p.m.) > > > Review request for mesos, Benjamin Bannier, Benjamin Mahler, Kapil Arya, > Michael Park, and Meng Zhu. > > > Bugs: MESOS-8069 > https://issues.apache.org/jira/browse/MESOS-8069 > > > Repository: mesos > > > Description > ------- > > When listing role "a", resources allocated to role "a/b" are added to > those allocated to role "a". These changes affect both, the "/roles" > endpoint as well as the V1 HTTP Call "GET_ROLES". > Adds dynamic hierarchical role tracking to the master. > > > Diffs > ----- > > include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 > include/mesos/v1/master/master.proto > ddb28f96b2a3a439bb9a829995a9a3015f65ba43 > include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 > src/master/http.cpp 34c9023906eca94965acc994f20e888c1f47b962 > src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 > src/master/master.cpp 18382fa93fd0c59e641e00f2028ac1ae2e67c01c > src/master/quota_handler.cpp 21bafd0064e9397f88185eaf687a58f85da94e2c > src/master/weights_handler.cpp 1053652804a8fc6af31a3b8a9f632f836c897fa9 > src/tests/api_tests.cpp dd8e221d8fd1b2a241505345337897e4ee4a6347 > src/tests/role_tests.cpp a609ed27ef828ca82efc0d269669cda92e5f50a1 > > > Diff: https://reviews.apache.org/r/65939/diff/2/ > > > Testing > ------- > > make check > > > Thanks, > > Till Toenshoff > >