> On June 13, 2013, 11:44 p.m., Vinod Kone wrote: > > src/master/master.cpp, lines 248-251 > > <https://reviews.apache.org/r/11206/diff/6/?file=304917#file304917line248> > > > > Why do we always want to allow a default role?
We did this to minimize the impact of these changes on people who don't care about roles or hierarchical allocation, but I agree that we don't always want a default role. Maybe the solution is to add the default role if no --role flag is present, and then allow people who use roles to turn on the default role by including a '*' in their --roles flag. > On June 13, 2013, 11:44 p.m., Vinod Kone wrote: > > src/master/master.cpp, lines 598-602 > > <https://reviews.apache.org/r/11206/diff/6/?file=304917#file304917line598> > > > > Don't we have to check this in re-register too? Good point. This also brings up the question of what we should do if a framework tries to reregister with a different role. We could check this by sending FrameworkInfos from the slaves to the master when they reregister, but that gets complicated if a framework reregisters before any slaves that it has tasks running on reregister, or if a framework that doesn't have any currently running tasks, and therefore none of the slaves have a copy of its FrameworkInfo, reregisters. > On June 13, 2013, 11:44 p.m., Vinod Kone wrote: > > src/tests/allocator_tests.cpp, lines 79-81 > > <https://reviews.apache.org/r/11206/diff/6/?file=304919#file304919line79> > > > > Can we add tests for when frameworks connect with a role that is > > known/unknown to the master? That's included in the 'tests' review later in this review chain. > On June 13, 2013, 11:44 p.m., Vinod Kone wrote: > > src/master/master.cpp, line 259 > > <https://reviews.apache.org/r/11206/diff/6/?file=304917#file304917line259> > > > > why don't we call the member 'role' instead of 'name'? > > > > I think it is a bit confusing that we call the flag tokens 'role's, but > > we have a class called 'Role' and an attribute of RoleInfo called 'name' > > which is actually a 'role'. > > I think that 'name' is cleaner, since it's in line with the way that Resource, Attribute, FrameworkInfo, ExecutorInfo, and TaskInfo all have 'name' fields. - Thomas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11206/#review21872 ----------------------------------------------------------- On June 13, 2013, 7:24 p.m., Thomas Marshall wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/11206/ > ----------------------------------------------------------- > > (Updated June 13, 2013, 7:24 p.m.) > > > Review request for mesos and Benjamin Hindman. > > > Description > ------- > > Previously when we were doing hierarchical allocation by user, we created and > removed user pools for allocation based on what users had frameworks > currently running in the cluster. However, with the role abstraction it makes > sense to have more persistence than that, especially once we add weights - if > you set the weight for a role, you want the allocator to remember it even if > there aren't any frameworks for that role currently running. > > So, I decided that it made sense to create a concept of specific roles that > are allowable in the cluster. With this patch, its only possible to pass > roles in to the master as a command line flag (to ease what I assume will be > the common case - clusters with relatively static sets of roles), but a > future wdrf patch will add http endpoints to add, remove, and update roles. > > This patch also enforces that frameworks register with valid role (this won't > affect people who don't care about roles, since there's always the "*" role, > which is the default for frameworks that don't specify a role). > > > This addresses bug MESOS-504. > https://issues.apache.org/jira/browse/MESOS-504 > > > Diffs > ----- > > src/master/allocator.hpp 78c75bb > src/master/flags.hpp f4ce8c1 > src/master/hierarchical_allocator_process.hpp 1048a28 > src/master/master.hpp 86c5232 > src/master/master.cpp 60c6d4f > src/messages/messages.proto 2c196ee > src/tests/allocator_tests.cpp 32f0a90 > src/tests/allocator_zookeeper_tests.cpp 1daaecd > src/tests/mesos.hpp fca41aa > src/tests/resource_offers_tests.cpp 3d5f02d > > Diff: https://reviews.apache.org/r/11206/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Thomas Marshall > >
