> On March 6, 2013, 7:23 p.m., Ben Mahler wrote: > > src/master/hierarchical_allocator_process.hpp, line 529 > > <https://reviews.apache.org/r/6665/diff/8/?file=264106#file264106line529> > > > > Looks to me like allocatable can just be a function on the slave struct: > > > > bool allocatable() const { return whitelisted && > > isSufficient(available); } > > > > Seems cleaner, no? That way we only have to update the whitelisted bit > > here and elsewhere. > > > > This loop would then become: > > > > foreachkey (const SlaveID& slaveId, slaves) { > > slave[slaveId].whitelisted = isWhitelisted(slaveId); > > } > > Thomas Marshall wrote: > I implemented this the way that you suggested because it seems > reasonable, but I just wanted to note that this means that the title of this > review is now meaningless and this patch only accomplishes restructuring the > allocator to have clearer code and does not do anything about speeding > allocations up, since we are back to checking whitelisting and resource > minimums on all slaves for every allocation now (again, as per your > suggestions). > > I'm assuming that your relative lack of concern about efficiency issues > means that Twitter isn't experiencing any problems with the allocator running > too slowly, so I'm perfectly fine with this. > > Ben Mahler wrote: > Great! I don't quite get your comment here. We do indeed now check > resource minimums but "checking whitelisting" is *only* checking the newly > added whitelisted bit, rather than the hashset, right? (are you referring to > line 699?) > > Thomas Marshall wrote: > Yes, I'm referring to line 699, which now essentially does the work of > the giant for loop at the beginning of allocate() eliminating which was the > original goal of this patch. It's true that we've changed checking whitelist > status from a call to isWhitelisted (which checks against the hashset) to > checking the whitelisted bit, but we've reverted from not needing to check > resources (because membership in the list we were iterating through indicated > sufficient resources), to checking a bit for sufficient resources, back to > actually pulling out and checking resources for each slave. > > Again, this isn't a problem for me, and I'll update the title and > description accordingly.
Also, for more background, we undertook some performance analysis at twitter on a large simulated cluster. A large amount of time (~20%) was spent in the allocator, from which significant time was spent working with the resources. Check out this pprof call graph: https://dl.dropbox.com/u/8265424/pprof.svg - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6665/#review17482 ----------------------------------------------------------- On March 12, 2013, 8:58 p.m., Thomas Marshall wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6665/ > ----------------------------------------------------------- > > (Updated March 12, 2013, 8:58 p.m.) > > > Review request for mesos and Benjamin Hindman. > > > Description > ------- > > There has been an unfortunate trend in the HierarchicalAllocatorProcess code > to use lots of hashmaps between slaveIds or frameworkIds and some attribute > of those frameworks/slaves. This patch collapses those into two hashmaps, one > for slaves and one for frameworks, mapping ids to structs containing the > appropriate attributes. The patch also introduces an allocator namespace. > > > Diffs > ----- > > src/local/local.hpp 2633d25 > src/local/local.cpp 3402029 > src/master/allocator.hpp b68b67d > src/master/drf_sorter.hpp 79566cc > src/master/drf_sorter.cpp fe31a00 > src/master/hierarchical_allocator_process.hpp c1d6f54 > src/master/main.cpp ca0abec > src/master/master.hpp c9b4b3f > src/master/master.cpp 814a6e1 > src/master/sorter.hpp 73db6b1 > src/tests/allocator_tests.cpp b953cd1 > src/tests/allocator_zookeeper_tests.cpp 3f70202 > src/tests/fault_tolerance_tests.cpp 61509f1 > src/tests/gc_tests.cpp fbdd6d6 > src/tests/master_detector_tests.cpp 787ba19 > src/tests/master_tests.cpp 2ba14fc > src/tests/resource_offers_tests.cpp 44eaf0d > src/tests/sorter_tests.cpp 61e6038 > src/tests/utils.hpp d3efa58 > third_party/libprocess/third_party/stout/include/stout/stringify.hpp > 136316d > > Diff: https://reviews.apache.org/r/6665/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Thomas Marshall > >
