> On Oct. 25, 2012, 4:53 p.m., Benjamin Hindman wrote: > > src/master/allocator.hpp, line 39 > > <https://reviews.apache.org/r/7720/diff/3/?file=179757#file179757line39> > > > > Why not just put this forward declaration down in in the > > mesos::internal block below?
It felt a bit ugly to stick it below. But, opening and closing namespace for fwd declarations is also not pretty. i'm fine either way. Lets stick with one style. I will follow the former style for now. > On Oct. 25, 2012, 4:53 p.m., Benjamin Hindman wrote: > > src/master/allocator.hpp, line 128 > > <https://reviews.apache.org/r/7720/diff/3/?file=179757#file179757line128> > > > > Do you want people to extend Allocator? No, dropped virtual. But, that's not enough to stop inheritance right? I need a public factory method and make the constructor private. > On Oct. 25, 2012, 4:53 p.m., Benjamin Hindman wrote: > > src/master/allocator.hpp, line 170 > > <https://reviews.apache.org/r/7720/diff/3/?file=179757#file179757line170> > > > > This isn't necessary because you defined a constructor above. But what > > about copying and assigning? done > On Oct. 25, 2012, 4:53 p.m., Benjamin Hindman wrote: > > src/master/allocator.cpp, line 29 > > <https://reviews.apache.org/r/7720/diff/3/?file=179758#file179758line29> > > > > 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!? as discussed offline, this is a trade-off between possible increase in compilation time vs run-time performance. but, i do agree that this file is less likely to be touched in the future (famous last words?) and hence am ok with pulling the impl up into the hpp. > On Oct. 25, 2012, 4:53 p.m., Benjamin Hindman wrote: > > src/master/allocator.cpp, line 145 > > <https://reviews.apache.org/r/7720/diff/3/?file=179758#file179758line145> > > > > So who deletes the HierarchicalAllocatorProcess instance? good point. > On Oct. 25, 2012, 4:53 p.m., Benjamin Hindman wrote: > > src/master/hierarchical_allocator_process.hpp, line 490 > > <https://reviews.apache.org/r/7720/diff/3/?file=179761#file179761line490> > > > > Eh, it looked less jagged before; I was quite happy with how Thomas > > wrapped these. done > On Oct. 25, 2012, 4:53 p.m., Benjamin Hindman wrote: > > src/tests/allocator_tests.cpp, line 304 > > <https://reviews.apache.org/r/7720/diff/3/?file=179767#file179767line304> > > > > I think when we use 'DoAll' it's easier to read each thing that we're > > doing by wrapping, so: > > > > Trigger(...). > > Return(Nothing) agreed, done. here and everywhere else in this file! > On Oct. 25, 2012, 4:53 p.m., Benjamin Hindman wrote: > > src/tests/allocator_tests.cpp, line 836 > > <https://reviews.apache.org/r/7720/diff/3/?file=179767#file179767line836> > > > > Why 6.0? my bad, forgot to remove it after my debugging session. > On Oct. 25, 2012, 4:53 p.m., Benjamin Hindman wrote: > > src/tests/allocator_zookeeper_tests.cpp, line 153 > > <https://reviews.apache.org/r/7720/diff/3/?file=179768#file179768line153> > > > > This is going to conflict with my pending review. i copied it based on your review :) > On Oct. 25, 2012, 4:53 p.m., Benjamin Hindman wrote: > > src/tests/utils.hpp, line 258 > > <https://reviews.apache.org/r/7720/diff/3/?file=179774#file179774line258> > > > > 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. In general, I dont like the fact that we are directly making calls to a libprocess process (real) from outside it (from MockAllocatorProcess). The real process is/might've been written based on the assumption that its public functions are called via dispatch. In other words, no two public functions would be expected to be executed at the same time, or no two external entities can be executing the same function at the same time. Does that make sense? - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7720/#review12764 ----------------------------------------------------------- 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 > >
