----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11206/#review21726 -----------------------------------------------------------
include/mesos/mesos.proto <https://reviews.apache.org/r/11206/#comment44908> After reviewing this code I'm starting to like the general purposeness of 'weight' and I think our comments should emphasize this. In particular, we should basically explain 'weight' as a way to "prioritize" roles, and that certain allocation algorithms, such as DRF, can use the weight information to allocate appropriately. Doing so means that future algorithms that might also want to "prioritize" roles can use weights as well. How does that sound? include/mesos/mesos.proto <https://reviews.apache.org/r/11206/#comment44906> Do we need to expose this to framework developers? This seems like an internal structure for implementation purposes. src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/11206/#comment44899> s/framewworks/frameworks/ src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/11206/#comment44900> s/CHECK (/CHECK(/ src/master/master.hpp <https://reviews.apache.org/r/11206/#comment45021> 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. src/master/master.cpp <https://reviews.apache.org/r/11206/#comment44895> 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). src/master/master.cpp <https://reviews.apache.org/r/11206/#comment44896> Two things: (1) Let's be nice to our users and fail early here via EXIT(1) rather than forcing them to read through the log file to find out that they had malformed flag input. We should probably do this in more places in the codebase, but no reason not to do this for new places. (2) Let's be a bit more explicit for our users and explain that the either (a) the input was not of the form 'foo=1,bar=2' or (b) a role was not included in the 'roles' flag. src/master/master.cpp <https://reviews.apache.org/r/11206/#comment44897> 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? src/master/master.cpp <https://reviews.apache.org/r/11206/#comment44901> roles[role] = new Role(roleInfo); src/master/master.cpp <https://reviews.apache.org/r/11206/#comment44902> Why the bool return code? src/tests/allocator_tests.cpp <https://reviews.apache.org/r/11206/#comment44898> Why the extra newline here (and in the rest of the tests in this diff)? This is not the convention in the rest of the tests. - Benjamin Hindman On June 12, 2013, 5:49 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, 5:49 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). > > > 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 > >
