> On March 12, 2013, 6:09 p.m., Ben Mahler wrote: > > Thanks for the patience Thomas, I think you've managed to simplify the > > allocator code quite nicely here! > > > > As a note, I think we should clean up the crazy filter pointer ownership as > > well, but in another review. > > Thomas Marshall wrote: > Agreed about the filters. It'll go on my allocator todo list.
Perfect! > On March 12, 2013, 6:09 p.m., Ben Mahler wrote: > > src/master/hierarchical_allocator_process.hpp, line 769 > > <https://reviews.apache.org/r/6665/diff/9/?file=269278#file269278line769> > > > > So I was looking at resources.hpp, and these are intensive operations > > at the moment, perhaps less so if / when we make resources first class > > fields in the protobufs. > > > > Also, there are duplicate operator implementations defined there, > > unless I'm seeing things! > > Thomas Marshall wrote: > This particular line, 'slaves[slaveId].available -= resources', is > deceptive since slaves[slaveId].available will always == resources and all > you're really wanting to do is zero out the resources in > slaves[slaveId].available. There's not currently an easy way of doing this, > but its an operation that I use a lot in implementing static allocations, so > I'll be adding some sort of '.clear()' type function to Resources in a later > patch and I'll deal with this accordingly then, if that's okay. > > When you say duplicate operator implementations, are you maybe confusing > the singular '-= Resource' and plural '-= Resources' functions? Yes, an more efficient way to clear the resources would be great! For the duplicates, I see the following duplicated signatures (each has two implementations..?): Resources operator + (const Resources& that) const Resources operator - (const Resources& that) const Resources& operator += (const Resources& that) Resources& operator -= (const Resources& that) - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6665/#review17739 ----------------------------------------------------------- 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 > >
