----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6665/#review16911 -----------------------------------------------------------
Higher level comment: It's unfortunate that, with this change, the allocator now has _3_ collections based on the slave id. Can you consider using one collection that keeps an appropriate struct for the slave information? This would avoid the need to keep this new hashset, the lookup would be transformed into checking a field on the slave information struct. src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment35883> s/offerable/sufficient I think the semantics you're capturing with this function are: Are the resources sufficient? if (sufficient(resources)) reads intuitively to me src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment35884> Likewise: // All whitelisted slaves with sufficient resources. src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment35885> why the change here? src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment35886> const &? - Ben Mahler On Feb. 21, 2013, 2:10 a.m., Thomas Marshall wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6665/ > ----------------------------------------------------------- > > (Updated Feb. 21, 2013, 2:10 a.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 > >
