----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7720/#review12764 -----------------------------------------------------------
Ship it! src/master/allocator.hpp <https://reviews.apache.org/r/7720/#comment27290> Why not just put this forward declaration down in in the mesos::internal block below? src/master/allocator.hpp <https://reviews.apache.org/r/7720/#comment27289> Not in headers please, even though this is scoped within namespaces. src/master/allocator.hpp <https://reviews.apache.org/r/7720/#comment27291> I liked the formatting before better, I think it's more readable (much less whitespace, much less jaggedness). src/master/allocator.hpp <https://reviews.apache.org/r/7720/#comment27292> Do you want people to extend Allocator? src/master/allocator.hpp <https://reviews.apache.org/r/7720/#comment27293> This isn't necessary because you defined a constructor above. But what about copying and assigning? src/master/allocator.hpp <https://reviews.apache.org/r/7720/#comment27294> s/allocatorProcess/process/ src/master/allocator.cpp <https://reviews.apache.org/r/7720/#comment27314> If we include all of this implementation in the bottom of allocator.hpp than we will get the dispatch calls inlined, effectively giving us the exact same performance characteristics as before but with the much nicer syntax. Sound good!? src/master/allocator.cpp <https://reviews.apache.org/r/7720/#comment27296> I don't think this "factory" is needed right now (or maybe ever). Please kill (see comment below for why). src/master/allocator.cpp <https://reviews.apache.org/r/7720/#comment27295> So who deletes the HierarchicalAllocatorProcess instance? src/master/drf_sorter.cpp <https://reviews.apache.org/r/7720/#comment27297> I've just been including "logging/logging.hpp" so as we add new abstractions we only have to include one thing (and then everything is consistent). src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/7720/#comment27299> Please wrap 'resources' then too. src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/7720/#comment27301> Eh, it looked less jagged before; I was quite happy with how Thomas wrapped these. src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/7720/#comment27300> Ditto above comment. src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/7720/#comment27302> Ditto comment above. src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/7720/#comment27303> Ditto comment above. src/master/master.hpp <https://reviews.apache.org/r/7720/#comment27304> Can kill newline. src/master/master.cpp <https://reviews.apache.org/r/7720/#comment27305> Pretty! src/master/master.cpp <https://reviews.apache.org/r/7720/#comment27306> Oooh! src/master/master.cpp <https://reviews.apache.org/r/7720/#comment27307> Aaaaahhhh! src/master/master.cpp <https://reviews.apache.org/r/7720/#comment27308> Okay, I'll stop now. ;) src/tests/allocator_tests.cpp <https://reviews.apache.org/r/7720/#comment27309> I think when we use 'DoAll' it's easier to read each thing that we're doing by wrapping, so: Trigger(...). Return(Nothing) src/tests/allocator_tests.cpp <https://reviews.apache.org/r/7720/#comment27310> Why 6.0? src/tests/allocator_zookeeper_tests.cpp <https://reviews.apache.org/r/7720/#comment27311> This is going to conflict with my pending review. src/tests/allocator_zookeeper_tests.cpp <https://reviews.apache.org/r/7720/#comment27312> These too. :( src/tests/utils.hpp <https://reviews.apache.org/r/7720/#comment27313> Why do you need to do this? Provided that they were dispatched to MockAllocatorProcess directly invoking should be fine. Since this is a process, any direct calls to it should be forbidden anyway. - Benjamin Hindman On Oct. 25, 2012, 1:58 a.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7720/ > ----------------------------------------------------------- > > (Updated Oct. 25, 2012, 1:58 a.m.) > > > Review request for mesos, Benjamin Hindman, Thomas Marshall, and Ben Mahler. > > > Description > ------- > > Specifically: > > --> Allocator writers don't need to do extra work to make their allocator > async. This is guaranteed/enforced by the Allocator interface. > > --> Inside master, the allocator calls looks like method calls instead of > dispatches. > > > Diffs > ----- > > src/local/local.hpp cd0483c72f919e08a67b4eeaebef3b62b0ff9798 > src/local/local.cpp 67c7a7c260d2ea610fb64b491f9bf7a22ec53aac > src/master/allocator.hpp 9fd790564d1361b9bdb2111f29348a631f66c831 > src/master/allocator.cpp 3f822912a4e0c50c1612eac98ae85447aae023d4 > src/master/drf_sorter.hpp 9c43ba32c6ed04fd6960524b5925c5c27c3e4d2d > src/master/drf_sorter.cpp 0ab4a14ca0bfb721de00e29d993fd4aa08a27ac5 > src/master/hierarchical_allocator_process.hpp > 266d339bdb72f8c63288d91c1f514da07ee9acf2 > src/master/main.cpp 813ef7de8e57b1cbd688e6e34e360c7b191ed641 > src/master/master.hpp 146af017bbb6da9bd44acb53a4f1ee0ffbedd64f > src/master/master.cpp 82e4dc704e5c67ec178bd058934896328308d868 > src/master/sorter.hpp 80d0893b1f988898e40ae3a2f6f19a117ad5f72d > src/sched/sched.cpp 5a4e594a3a8a2089cf74e7e418f2e7bde4c03e9d > src/tests/allocator_tests.cpp ed52ff61b1820cb3f5f7f2ae421fb3f84a85939a > src/tests/allocator_zookeeper_tests.cpp > bd408c22df5edc3684791e509ce1a4c210553922 > src/tests/fault_tolerance_tests.cpp > 558bf41ab6f4ba319ec3b23f3347f98e235260f9 > src/tests/gc_tests.cpp 1de240579a1cc6af8738d2ffb5da1b3e01c4b19e > src/tests/master_detector_tests.cpp > fb32af8130d3f359eecba5389c2e42c247d6b70e > src/tests/master_tests.cpp 37e97e0a58a0128579f155183b7e769ab4e1b452 > src/tests/resource_offers_tests.cpp > c50da38508694f956ad104fb160896eb9b9d5fea > src/tests/utils.hpp 4066c02b23db15310104955465c4e4afd7bcad5b > > Diff: https://reviews.apache.org/r/7720/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
