> 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?)
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. - Thomas ----------------------------------------------------------- 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 > >
