> On Jan. 14, 2015, 11:36 p.m., Niklas Nielsen wrote: > > Very nice refactor, Alex! > > > > RR comment: This patch touches quite a few files and could be made more > > consumable if you broke it up a bit further. > > Are you familiar with support/post-reviews.py ? > > > > General: A high-level question is what to call the default allocator and > > whether to move it to its own allocators directory (where it can exist > > along with other allocator implementations).
The RR is a bit big, I agree, but that's the atomic change that breaks neither build, nor tests. An alternative was to cut this into smaller RRs with the assumption that all will be committed together. I try to avoid the latter approach, since a single commit does break the build, which may influence several git workflows, like bisecting for a bug. I'll submit additional patches to finish the refactoring as separate RR to keep this patch as small as possible. > On Jan. 14, 2015, 11:36 p.m., Niklas Nielsen wrote: > > src/master/allocator.hpp, line 61 > > <https://reviews.apache.org/r/29890/diff/1/?file=821527#file821527line61> > > > > This will be the public API and it would be great to have comments on > > all calls to be implemented. Can we leave a todo, so we get that fixed? TODO added, JIRA filed: https://issues.apache.org/jira/browse/MESOS-2224 > On Jan. 14, 2015, 11:36 p.m., Niklas Nielsen wrote: > > src/master/allocator.hpp, lines 144-145 > > <https://reviews.apache.org/r/29890/diff/1/?file=821527#file821527line144> > > > > Let's do this now. Let's do it in a separate RR: https://reviews.apache.org/r/29931/ > On Jan. 14, 2015, 11:36 p.m., Niklas Nielsen wrote: > > src/master/allocator.hpp, line 147 > > <https://reviews.apache.org/r/29890/diff/1/?file=821527#file821527line147> > > > > How about calling ActorAllocator the DRFAllocator or MesosAllocator > > (similiar to the default containerizer)? Think we could come up with a more > > precise name than ActorAllocator. Hopefully, other allocators are also > > actors :) I like `MesosAllocator`, `DRFAllocator` is too specific. This class is a wrapper for libprocess process based allocators, the allocation algorithm can be arbitrary. The base `Allocator` doesn't require implementations to be [libprocess based] actors any more. > On Jan. 14, 2015, 11:36 p.m., Niklas Nielsen wrote: > > src/tests/cluster.hpp, lines 100-101 > > <https://reviews.apache.org/r/29890/diff/1/?file=821530#file821530line100> > > > > How is this different from before? No, just postponed it for a separate review to keep things simple: https://reviews.apache.org/r/29926/ > On Jan. 14, 2015, 11:36 p.m., Niklas Nielsen wrote: > > src/tests/master_allocator_tests.cpp, lines 79-82 > > <https://reviews.apache.org/r/29890/diff/1/?file=821532#file821532line79> > > > > So the patch will make tests flaky without a stop() method? Yes, I would assume that, though I was unable to reproduce the test failure with `--gtest_repeat=10000 --gtest_break_on_failure`. Nevertheless, I restored the logic here: https://reviews.apache.org/r/29927/ > On Jan. 14, 2015, 11:36 p.m., Niklas Nielsen wrote: > > src/tests/resource_offers_tests.cpp, lines 40-41 > > <https://reviews.apache.org/r/29890/diff/1/?file=821535#file821535line40> > > > > Any particular reason you removed this? This type used only once, so I opted for a fully-qualified name at the point where the type is used. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29890/#review68160 ----------------------------------------------------------- On Jan. 14, 2015, 6:51 p.m., Alexander Rukletsov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29890/ > ----------------------------------------------------------- > > (Updated Jan. 14, 2015, 6:51 p.m.) > > > Review request for mesos, Bernd Mathiske and Niklas Nielsen. > > > Bugs: MESOS-2213 > https://issues.apache.org/jira/browse/MESOS-2213 > > > Repository: mesos-git > > > Description > ------- > > Introduce a basic Allocator interface that every allocator should implement. > This interface does not require allocators to be based on libprocess process. > For allocators they do implement allocation logic via an internal libprocess > process, a special wrapper is provided. Allocator uses and tests are updated > to use Allocator type instead of AllocatorProcess. > > [WIP]: more refactoring will be done either in an update to this RR or in a > separate RR. > > > Diffs > ----- > > src/local/local.cpp 76e73a4 > src/master/allocator.hpp 224569a > src/master/hierarchical_allocator_process.hpp ccd37b4 > src/master/main.cpp 193d53f > src/tests/cluster.hpp 74cedb3 > src/tests/hierarchical_allocator_tests.cpp 7c05123 > src/tests/master_allocator_tests.cpp 2430622 > src/tests/mesos.hpp 591134b > src/tests/mesos.cpp 3b98c69 > src/tests/resource_offers_tests.cpp d098e70 > src/tests/slave_recovery_tests.cpp 809822e > > Diff: https://reviews.apache.org/r/29890/diff/ > > > Testing > ------- > > make distcheck (Ubuntu, OS X) > > > Thanks, > > Alexander Rukletsov > >
