----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30130/#review69089 -----------------------------------------------------------
What does it mean to kill code? ;) How about s/Killed/Removed/ in the title and description? Looks good, just some minor comments and we should be good to go. include/mesos/resources.hpp <https://reviews.apache.org/r/30130/#comment113694> How are you going to use this one? It seems like we will normally have a `RepeatedPtrField<Offer::Operation>`, do you want that instead? Or do you want a templatized version? ``` template <typename Iterable> Try<Resources> apply(const Iterable<Offer::Operation>& operations) const; ``` src/common/resources.cpp <https://reviews.apache.org/r/30130/#comment113697> How about the following to be a bit more explicit and consistent: ``` if (!volume.has_disk()) { return Error("Invalid CREATE Operation: Expecting 'disk' to be present"); } else if (!volume.disk().has_persistence)) { return Error("Invalid CREATE Operation: Expecting 'persistence' to be present"); } ``` Could you use this pattern througout this method for all Error returns? ``` "Invalid X Operation: <reason>" ``` Some of the existing errors don't include the operation type. src/common/resources.cpp <https://reviews.apache.org/r/30130/#comment113703> Ditto here for splitting these two cases apart. src/common/resources.cpp <https://reviews.apache.org/r/30130/#comment113705> Shouldn't this be printing the number of the type? As is it will just print the empty string if someone sets this incorrectly. Or does protobuf prevent an invalid enum value..? src/common/resources.cpp <https://reviews.apache.org/r/30130/#comment113706> "check" src/master/allocator.hpp <https://reviews.apache.org/r/30130/#comment113688> How about 'updateAllocation' now that we avoid using "transformation"? src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/30130/#comment113686> What happened here? Mind leaving as is to make the diff smaller? - Ben Mahler On Jan. 21, 2015, 6:11 p.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30130/ > ----------------------------------------------------------- > > (Updated Jan. 21, 2015, 6:11 p.m.) > > > Review request for mesos, Ben Mahler, Michael Park, and Vinod Kone. > > > Repository: mesos-git > > > Description > ------- > > Killed Resources::Transformation in favor of using Offer::Operation. > > This patch is based on and is a replacement for > https://reviews.apache.org/r/29744/ > > > Diffs > ----- > > include/mesos/resources.hpp 7935e7f9bfe66d1900594dcdcb800c4593a3940f > src/common/resources.cpp 214e441fb86aa0c094c28ed5801089051468137b > src/master/allocator.hpp 224569a55eb15d9e7c63f2b571a1f9ae8cd4b6e6 > src/master/hierarchical_allocator_process.hpp > ccd37b4348c785ece890257f805777acbaf8d58c > src/tests/hierarchical_allocator_tests.cpp > 7c051232f2c70691a7789e0f3e94f0e4cba26f18 > src/tests/mesos.hpp 591134bab31da0a1cc224e332666d05f320d0f87 > src/tests/resources_tests.cpp b7c1ddfda952f2d6b7f82e90cfe7b6b0aafbc36a > src/tests/sorter_tests.cpp 56e5714c2ab97d0ac81d29e1acb1fbec15471489 > > Diff: https://reviews.apache.org/r/30130/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jie Yu > >
