> On June 12, 2013, 11:13 p.m., Benjamin Hindman wrote: > > src/master/master.hpp, lines 181-184 > > <https://reviews.apache.org/r/11206/diff/4/?file=301197#file301197line181> > > > > Some thoughts on the 'updateRole' and 'removeRole' "family" of > > functions. First, 'removeRole' is never called, so why add it? Second, the > > semantics around updating a role are unclear to me. If we update a role's > > weight, does hat mean that the allocator needs to take appropriate action > > immediately and re-allocate as necessary? I think there are a lot of > > questions around these semantics that I don't think we need to tackle right > > now. Why not just pass the list of roles to the allocator when we > > initialize it for now? That is, in initialize, construct a map of > > RoleInfo's, update those as appropriate based on --weights, save those to > > the master and pass them to the allocator? This seems simple and > > straightforward and doesn't introduce complexities that I think we should > > punt for now.
This is left over from before when there was an endpoint for updating roles. Removed. > On June 12, 2013, 11:13 p.m., Benjamin Hindman wrote: > > src/master/master.cpp, lines 399-402 > > <https://reviews.apache.org/r/11206/diff/4/?file=301198#file301198line399> > > > > You could clean this up using 'strings::pairs'. The caveat here is that > > it ignores malformed input. I like validating the input so I suggest: > > > > (1) Updating strings::pairs to return a Try<std::map<...>> and use it, > > outputting the error as necessary. > > > > (2) Continue to do it the way you are doing (but creating a Github > > issue at 3rdparty/stout would be nice). The problem with having strings::pairs return a Try is that the one place we are actually using it right now, process::http::Response::accepts, relies on it simply ignoring invalid pairs in order to work. I don't know what the right work around for this is, so I'll just leave it all as is for now. > On June 12, 2013, 11:13 p.m., Benjamin Hindman wrote: > > src/master/master.cpp, line 1700 > > <https://reviews.apache.org/r/11206/diff/4/?file=301198#file301198line1700> > > > > Yes, you've made sure the role is valid in Master::registerFramework, > > but no reason not to put a CHECK here for your fellow developers. In fact, > > you can even add a comment above the CHECK pointing to the validation in > > Master::registerFramework. Another question, what happens if a slave > > connects with a role that the master doesn't know about? I'm not sure what to do about the slave registering with an invalid role situation. Obviously we don't want a situation where the resources dedicated to the invalid role just silently sit there idle, so we could either have the master log an error and put the resources into the default pool, or we could create a slave registration error message to send back so that the slave can exit (this second option seems more in line with what you said about malformed --weights flags). - Thomas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11206/#review21726 ----------------------------------------------------------- On June 12, 2013, 11:40 p.m., Thomas Marshall wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/11206/ > ----------------------------------------------------------- > > (Updated June 12, 2013, 11:40 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 > ----- > > include/mesos/mesos.proto 8cbcd9a > 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/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 > >
