----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6665/#review17197 -----------------------------------------------------------
src/local/local.hpp <https://reviews.apache.org/r/6665/#comment36473> not yours, but can you adjust the formatting to be compliant with the new style? the params should be intended by 4 spaces and start from next line. src/master/allocator.hpp <https://reviews.apache.org/r/6665/#comment36474> I'm surprised the master:: qualification was necessary? src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment36475> initialize whitelisted and available here. src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment36478> Why did you have to write your own assignment operator here and below for framework? Is it just so that you can set 'initialized'? src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment36476> It might be just me, but I think swapping these variable names makes more sense? I understand you called it 'allocatable' because how its called in the allocator. At the least, I think available should be 'sufficient' to be consistent with the verb we use in the allocator. src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment36479> You probably need a check here CHECK(frameworks.contains(frameworkId)); src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment36480> ditto. src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment36481> we tend to put the operators at the end when we line wrap. if (slaves[slaveId].whitelisted && isSufficient(....)) src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment36482> again, i'm surprised you need this qualification. src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/6665/#comment36483> CHECK(frameworks.contains(framweworkId)? - Vinod Kone On Feb. 28, 2013, 1:42 a.m., Thomas Marshall wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6665/ > ----------------------------------------------------------- > > (Updated Feb. 28, 2013, 1:42 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/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 > >
