----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6665/#review17482 -----------------------------------------------------------
Thanks for the cleanup, this is looking much better! Sorry for all the back and forth =/ src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment37081> I like the defensiveness of the CHECKs, however, I think it's too much complexity, given we already use default constructors all over the place in our map values. So I think for Slave and Framework, let's 1. kill the 'initialized' member 2. one-line the 'resources()', 'hostname()', and 'user()' helpers (akin to the duration.hpp one-line methods) 3. have the default constructor have no initializer list (unless the compiler complains?) I think that will simplify this code quite a bit! src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment37077> const src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment37078> const src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment37079> const src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment37082> inline? frameworks[frameworkId] = Framework(frameworkInfo); src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment37083> const&? src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment37086> much nicer! But maybe we need to keep the comment as to why we don't delete the filters (in the Framework destructor) src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment37084> const&? src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment37087> Here as well, keep the comment on why we don't delete them. src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment37088> Inline? slaves[slaveId] = Slave(slaveInfo); Also, maybe you want to add whitelisted as an optional constructor parameter? src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment37092> Looks like stringify will work instead? LOG(INFO) << "Updated slave white list: " << strinfify(whitelist.get()); We have set utilities in stringify.hpp template <typename T> std::string stringify(const std::set<T>& set) { std::ostringstream out; out << "{ "; typename std::set<T>::const_iterator iterator = set.begin(); while (iterator != set.end()) { out << stringify(*iterator); if (++iterator != set.end()) { out << ", "; } } out << " }"; return out.str(); } src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment37096> 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); } src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment37099> else on the same line as closing brace src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment37095> ditto src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment37100> const&? src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment37101> const&? src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment37102> empty()? src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment37103> const&? src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment37105> !empty()? src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment37107> Supposing you made allocatable a function in Slave as I described above, it would now be returning false as expected right? Since isSufficient(slave.available) == false? src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment37076> Just an FYI, Vinod has introduced helpers to resources: Option<double> cpus = resources.cpus(); Option<double> mem = resources.mem(); But feel free to leave as is, since we don't currently have an optional default value in Option::get(). So using these helpers here is probably more code than what you're doing now. - Ben Mahler On March 1, 2013, 7:48 p.m., Thomas Marshall wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6665/ > ----------------------------------------------------------- > > (Updated March 1, 2013, 7:48 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/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 33a8ec8 > src/master/hierarchical_allocator_process.hpp 33e059c > 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 44d72ad > src/tests/allocator_zookeeper_tests.cpp 5f83dd7 > src/tests/fault_tolerance_tests.cpp 44eef03 > src/tests/gc_tests.cpp 411bcc0 > src/tests/master_detector_tests.cpp 5d09bab > src/tests/master_tests.cpp e140f40 > src/tests/resource_offers_tests.cpp 5981191 > src/tests/sorter_tests.cpp 61e6038 > src/tests/utils.hpp 4600c2f > > Diff: https://reviews.apache.org/r/6665/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Thomas Marshall > >
