> On Dec. 8, 2015, 3:20 p.m., Qian Zhang wrote: > > src/master/allocator/mesos/hierarchical.hpp, line 407 > > <https://reviews.apache.org/r/41075/diff/2/?file=1155876#file1155876line407> > > > > s/iff/if/
"iff" is what was intended (https://en.wikipedia.org/wiki/If_and_only_if#Origin_of_iff). > On Dec. 8, 2015, 3:20 p.m., Qian Zhang wrote: > > src/master/allocator/mesos/hierarchical.hpp, line 367 > > <https://reviews.apache.org/r/41075/diff/2/?file=1155876#file1155876line367> > > > > Why "Frameworks are removed from this map"? I think it should be "Role" > > removed from this map. Thanks, fixed. > On Dec. 8, 2015, 3:20 p.m., Qian Zhang wrote: > > src/master/master.hpp, line 1331 > > <https://reviews.apache.org/r/41075/diff/2/?file=1155879#file1155879line1331> > > > > Do we need to put a TODO here to mention that `validRoles` needs to be > > removed once "explicit roles" feature is deprecated? I believe at that > > time, we will just rely on ACL. In my opinion, a separate "TODO" is not needed -- the feature is marked as deprecated. When it is removed, all the code required to implement it can be removed. > On Dec. 8, 2015, 3:20 p.m., Qian Zhang wrote: > > src/master/master.cpp, lines 5842-5843 > > <https://reviews.apache.org/r/41075/diff/2/?file=1155880#file1155880line5842> > > > > Suggest to erase the role first, and then delete it, like how we remove > > offer in `Master::removeOffer()` Yeah, I considered doing it this way. Why do you think erasing the role first is better? (The `removeOffer` case is not quite analogous, because `delete`-ing the offer means `offer->id()` is no longer valid). - Neil ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41075/#review109321 ----------------------------------------------------------- On Dec. 8, 2015, 8:33 a.m., Neil Conway wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41075/ > ----------------------------------------------------------- > > (Updated Dec. 8, 2015, 8:33 a.m.) > > > Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg > Mann, and Yong Qiao Wang. > > > Bugs: MESOS-4085 > https://issues.apache.org/jira/browse/MESOS-4085 > > > Repository: mesos > > > Description > ------- > > Changed the behavior of the master when the `--roles` flag is NOT > specified. Previously, this would allow only the `*` role to be used. Now, > omitting `--roles` means that any role can be used. This is called "implicit > roles". Configuring which principals can perform operations as which roles > should be done using ACLs in the authorization system. > > Note that this changes the behavior of the system when `--roles` is not > specified. This is likely acceptable: if the operator didn't specify `--roles` > in prior versions of Mesos, they were likely not using roles or authorization > at > that time. > > Another minor behavioral change is that the "/roles" endpoint will now only > return results for currently "active" roles (those with one or more registered > frameworks). > > The `--roles` flag is now considered deprecated and will be removed in a > future > version of Mesos. > > > Diffs > ----- > > include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 > include/mesos/master/allocator.proto > 702f56f56c3b1331613cecf26522986f6b572f8c > src/master/allocator/mesos/allocator.hpp > 97ee80726ad155917811265a983258b0165d3451 > src/master/allocator/mesos/hierarchical.hpp > 99c742906874c30c39c159e58a65277ade3c07fd > src/master/allocator/mesos/hierarchical.cpp > 5da825a1d578a9ee40b4985378fddb3c5fb3b416 > src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83 > src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 > src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c > src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d > src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42 > src/tests/hierarchical_allocator_tests.cpp > fb214a829a57529d3f5c49730ae9733f53e622ca > > Diff: https://reviews.apache.org/r/41075/diff/ > > > Testing > ------- > > "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the > more likely role-related tests. > > TODOs: > > * Update documentation > * Add tests for allocation behavior for weights + implicit roles > * Add tests for quota + implicit roles? > > Notes: > > * There's two places where we use manual `new`/`delete` where a `unique_ptr` > would probably be nicer. I'm inclined to leave this as-is for now though > (making use of unique_ptr is a broader issue). > > > Thanks, > > Neil Conway > >