----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6665/#review17065 -----------------------------------------------------------
src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment36145> "Slave" would be a nicer name. I see that the allocator is inside the master namespace and so you have to deal with the fact that Slave is present inside the master header as well. Looking at the signatures on the allocator, it seems we don't need to know about the Slave struct from the master. Looks to me like all we need from the master namespace is 'Master'.. So we should either change the allocator to be namespaced inside "allocator" or further hide the master's structs (inside a namespace). Do other people rely on those structs? src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment36140> any reason not to do this in the initializer? src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment36146> Great, we managed to kill the allocatable map using this struct! I think we should push to reverse the trend in this file of using separate maps to track related things, which is why I'd like to see more logic pushed into this struct. Can you compare the performance to see if it's worth it to add the complexity of a separate hashset? Likewise for frameworks, I see maps from: FrameworkID -> user user -> FrameworkSorter It seems like these would be obviated by using a struct instead, right? For now, TODOs are fine if you don't want to take this on, but I don't think we'll want to add much more here before we clean this stuff up a bit. src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment36141> s/if/iff and line wrap to 70 columns src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment36142> I think you can kill the "i.e." part of this comment. Putting the logic of the function here seems redundant no? src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment36143> Line wrap at 70. src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment36144> In general we're trying to eliminate the concept of pointers in our maps, as it is implies the concept of ownership. The AllocatorSlave you created is copyable, right? src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment36148> Hm.. did you make this a pointer to avoid the need to add a default constructor? If so, adding a default is fine as long as there are sufficient CHECKs like the one above. - Ben Mahler On Feb. 25, 2013, 7:52 p.m., Thomas Marshall wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6665/ > ----------------------------------------------------------- > > (Updated Feb. 25, 2013, 7:52 p.m.) > > > Review request for mesos and Benjamin Hindman. > > > Description > ------- > > Currently, every time we do an allocation we have to traverse the entire list > of active slaves an check each one to see if its whitelisted and if it has > resources to allocate. This patch keeps a set of all slaves that meet those > requirements, and updates it when slaves are added/removed and when resources > are allocated/recovered. > > Timing comparisons, using test_framework on a local cluster on my laptop > (each number is an average over 10 tests): > > 100 slaves, 100 tasks > without patch: 5.82s > with patch: 5.773s > improvement of about 1% > > 1000 slaves, 100 tasks > without patch: 8.261s > with patch: 8.07s > improvement of about 2% > > Since this was a scalability issue, you'd presumably see a bigger improvement > in larger clusters but its difficult to simulate more than 1000 slaves or so > locally. If more convincing numbers are needed I can do some testing on EC2 > (if nothing else, I'm hoping we'll have a Jenkins build with automated > performance tests set up later this semester, so I can test this in the > process of setting that up), but at the very least it seems clear that the > patch improves the runtime complexity of the allocation algorithm and doesn't > slow allocations down even in small clusters. > > > Diffs > ----- > > src/master/hierarchical_allocator_process.hpp 33e059c > > Diff: https://reviews.apache.org/r/6665/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Thomas Marshall > >
