----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65939/#review202571 -----------------------------------------------------------
In order to make faster progress on this, can you split out the API change in front of this patch? We could get that committed without waiting for all this other logic: I think the API looks good, I just left some suggestions for comments. Also, feel free to pull out the CHANGELOG change after the logic so that it can be reviewed and given a ship it in isolation. CHANGELOG Lines 345-348 (patched) <https://reviews.apache.org/r/65939/#comment284386> Why are these under documentation? Should they be under API changes? CHANGELOG Lines 351-352 (patched) <https://reviews.apache.org/r/65939/#comment284387> Quote all three or quote none of them? include/mesos/mesos.proto Line 3479 (original), 3479 (patched) <https://reviews.apache.org/r/65939/#comment284398> Not for this branch, but could you file a ticket for getting quota added into this? include/mesos/mesos.proto Lines 3480-3481 (original), 3481-3490 (patched) <https://reviews.apache.org/r/65939/#comment284407> Can you document the difference between total and non-total? Not sure this will be clear to readers. include/mesos/mesos.proto Lines 3484 (patched) <https://reviews.apache.org/r/65939/#comment284390> The "Since 1.6" seems ambiguous: was it introduced thenn or deprecated then? How about: ``` // In 1.6 this field became deprecated in favor of // summing `allocated` + `offered`. ``` src/master/http.cpp Line 3418 (original), 3418 (patched) <https://reviews.apache.org/r/65939/#comment284403> I'm a little confused about the none case, what does it mean now? Are we changing the meaning? Would be good to have a comment about it. src/master/http.cpp Lines 3488-3495 (original), 3518-3525 (patched) <https://reviews.apache.org/r/65939/#comment284408> Not yours, but we probably want to follow up and avoid the copies in this path, as it gets rather expensive. src/master/http.cpp Lines 3646-3649 (original), 3677-3702 (patched) <https://reviews.apache.org/r/65939/#comment284411> All these variables mean an extra copy, do we need them? E.g. ``` role.mutable_allocated()->CopyFrom( role_->resources[Role::Account::ACCEPTED]); ``` src/master/master.hpp Lines 129-132 (patched) <https://reviews.apache.org/r/65939/#comment284406> (1) Why the hashmap instead of an array of size 2? ``` enum AccountingType { OFFERED, ALLOCATED }; std::array<Resources, 2> roleResources; std::array<Resources, 2> roleTreeResources; ``` (2) I think we should try to stick to "allocated" and "offered" instead of saying "accepted" here? (3) The names I suggested here might be clearer? Alternatively, the existing names are ok but would be good to add a comment about what the difference is between the two? src/master/master.hpp Lines 134-139 (patched) <https://reviews.apache.org/r/65939/#comment284414> Can you clarify that this is about non-default quota *and* weights? Seems to me this note belongs at the top right under "Information about a role."? src/master/master.hpp Lines 159-160 (patched) <https://reviews.apache.org/r/65939/#comment284416> Rather than "active" can you just directly say what "active" means? Active is already a word we use to mean a role can receive offers, can we avoid re-defining it? src/master/master.hpp Lines 185-187 (patched) <https://reviews.apache.org/r/65939/#comment284418> Implicit is the opposite of active? This seems confusing? src/master/master.hpp Lines 198-199 (patched) <https://reviews.apache.org/r/65939/#comment284417> This doesn't seem very intuitive to me, for example, shouldn't there be an entry if there are resources allocated to a role, even if there isn't a registered framework? Or is the comment wrong? Can't the user already figure out if it has frameworks by getting the role and counting the frameworks? src/master/master.hpp Lines 204 (patched) <https://reviews.apache.org/r/65939/#comment284420> Maybe topLevelRoles? src/master/master.cpp Lines 417-420 (patched) <https://reviews.apache.org/r/65939/#comment284434> We can do this later, but another spot for avoiding copies :) src/master/master.cpp Lines 429-433 (patched) <https://reviews.apache.org/r/65939/#comment284433> Why not just return nullptr for missing role? ``` return roleMap.get(role).getOrElse(nullptr); ``` That seems to be more in-line with our "get" style functions in the master? src/master/master.cpp Lines 448-452 (patched) <https://reviews.apache.org/r/65939/#comment284448> How about: // First add the role to the tree if necessary. Any // missing roles along the walk down will be added. ... // Now walk up the tree and add the framework to roles. src/master/master.cpp Lines 471 (patched) <https://reviews.apache.org/r/65939/#comment284443> Can you remove this? src/master/master.cpp Lines 473-476 (patched) <https://reviews.apache.org/r/65939/#comment284444> Can you std::move it into the map? src/master/master.cpp Lines 501 (patched) <https://reviews.apache.org/r/65939/#comment284438> It's a little weird to mix hasRole and direct map access? For example, I would expect direct map access or the public api: ``` Role* r = getRole(role); return r != nullptr && r->frameworks.contains(frameworkId); ``` Or: ``` auto r = roleMap.find(role); return r != roleMap.end() && r->second.frameworks.contains(frameworkId); ``` src/master/master.cpp Lines 428-429 (original), 513-525 (patched) <https://reviews.apache.org/r/65939/#comment284449> // First walk up the tree erasing the framework. src/master/master.cpp Lines 527 (patched) <https://reviews.apache.org/r/65939/#comment284450> // Clean up the role if empty, and walk up the tree // cleaning up any other roles that have become empty. src/master/master.cpp Lines 430-432 (original), 532-542 (patched) <https://reviews.apache.org/r/65939/#comment284456> how about breaking instead of returning? ``` if (!current->children.empty() || !current->frameworks.empty()) { break; } ``` src/master/master.cpp Lines 430-433 (original), 533-544 (patched) <https://reviews.apache.org/r/65939/#comment284454> can you remove these vlogs? src/master/master.cpp Lines 436-458 (original), 562-597 (patched) <https://reviews.apache.org/r/65939/#comment284428> Doing these loops on a resource-by-resource basis will be obscenely expensive :) We should mandate and check that these calls are in the scope of a single role. src/master/master.cpp Line 440 (original), 569 (patched) <https://reviews.apache.org/r/65939/#comment284429> Can't this be null? src/master/weights_handler.cpp Line 297 (original), 297 (patched) <https://reviews.apache.org/r/65939/#comment284425> I think this code ends up more intuitive if it just directly does what it says? ``` Role* role_ = master->roles.getRole(role); if (role_ != nullptr && !role_->framweorks.empty()) { rescind = true; break; } ``` - Benjamin Mahler On May 5, 2018, 2:14 p.m., Till Toenshoff wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65939/ > ----------------------------------------------------------- > > (Updated May 5, 2018, 2:14 p.m.) > > > Review request for mesos, Benjamin Bannier, Benjamin Mahler, Kapil Arya, > Michael Park, and Meng Zhu. > > > Bugs: MESOS-8069, MESOS-8789 and MESOS-8790 > https://issues.apache.org/jira/browse/MESOS-8069 > https://issues.apache.org/jira/browse/MESOS-8789 > https://issues.apache.org/jira/browse/MESOS-8790 > > > 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. > Deprecates the `resources` member of the `Role` Message in mesos.proto > (V0 + V1), replacing it by `allocated` and `offered`. Follows that > deprecation on role-related endpoints while adding resource accounting > for `[total_]allocated` and `[total_]offered` to "/roles" as well as > "GET_ROLES". > > > Diffs > ----- > > CHANGELOG 0254044808bc04014803ad36aebc999b4ccea816 > include/mesos/master/master.proto 54f84120728eea7995422b9c356ed67e5b054623 > include/mesos/mesos.proto 463e6adcdf14115b2f21270bb1fd9c45e1b67cc3 > include/mesos/v1/master/master.proto > 12f019d8d0902e212d4a3706fe2310b514d0d183 > include/mesos/v1/mesos.proto 8eaad9c4b2a9cdd527922a004b0f7dd0dc58a7f1 > src/master/http.cpp 77cf47a898a7b4fc3b548373fcf0f6232e39a369 > src/master/master.hpp 76e7763a99972f686af9d7eed08b6b6b1db23b0f > src/master/master.cpp 3b5d2eba3f602f68a6bb1e00444b01fb58a1bfc2 > src/master/quota_handler.cpp 21bafd0064e9397f88185eaf687a58f85da94e2c > src/master/weights_handler.cpp 1053652804a8fc6af31a3b8a9f632f836c897fa9 > > > Diff: https://reviews.apache.org/r/65939/diff/5/ > > > Testing > ------- > > make check > > > Thanks, > > Till Toenshoff > >
