----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30911/#review73743 -----------------------------------------------------------
Looking great Michael! I did find a regression here I think, please look it over carefully and let me know. src/common/resources.cpp <https://reviews.apache.org/r/30911/#comment120122> This is a little tricky to think through, how about: ``` if (role.isSome()) { return resource.role() == role.get(); } return resource.role() != "*"; ``` src/common/resources.cpp <https://reviews.apache.org/r/30911/#comment120123> Can you revert the change to the comment? I'm not sure "priority" is a helpful word here. By the way, I personally find (1) (2) style lists easier for the eye to scan. At the very least, we should indent these kinds of lists. src/common/resources.cpp <https://reviews.apache.org/r/30911/#comment120124> IMHO 'remaining' is what tells us that the approach used here is to pull out resources. Do you need to introduce this NOTE? src/common/resources.cpp <https://reviews.apache.org/r/30911/#comment120128> Should this be static? src/common/resources.cpp <https://reviews.apache.org/r/30911/#comment120125> Looks like a decent use case for '`auto`' given that the type of '`predicates`' is obvious and directly above this statement. src/master/allocator/mesos/hierarchical.hpp <https://reviews.apache.org/r/30911/#comment120130> Whoops! What happens when role is `"*"` now? The role sorter can wind up with `"*"` as a role, since FrameworkInfo uses that as the default and we don't reject frameworks with role `"*"`. It looks like with your change we're over-counting in this case. We should really add a NOTE about this gotcha. src/master/master.hpp <https://reviews.apache.org/r/30911/#comment120127> We're going to have to move this so the slave can leverage it. protobuf_utils.hpp is a mediocre place for now. Feel free to drop a TODO here and have jie move it in his slave validation change. src/master/master.hpp <https://reviews.apache.org/r/30911/#comment120126> No need for the code snippet (by the way your '`&`' is on the wrong side :)). src/master/master.hpp <https://reviews.apache.org/r/30911/#comment120129> should this be static? src/master/validation.cpp <https://reviews.apache.org/r/30911/#comment120131> ``` Resources volumes = Resources(resources).persistentVolumes(); foreach (const Resource& volume, volumes) { ... } ``` src/slave/slave.cpp <https://reviews.apache.org/r/30911/#comment120132> This is a bit hard to parse, can you do pull out a '`volumes`' variable (ditto from my comment above). src/slave/slave.cpp <https://reviews.apache.org/r/30911/#comment120133> Ditto here. Can you pull out a variable? src/slave/slave.cpp <https://reviews.apache.org/r/30911/#comment120134> Ditto here, mind pulling out a variable? src/tests/hierarchical_allocator_tests.cpp <https://reviews.apache.org/r/30911/#comment120135> Why the changes to this file? - Ben Mahler On Feb. 23, 2015, 10:12 p.m., Michael Park wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30911/ > ----------------------------------------------------------- > > (Updated Feb. 23, 2015, 10:12 p.m.) > > > Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, and > Vinod Kone. > > > Bugs: MESOS-2348 > https://issues.apache.org/jira/browse/MESOS-2348 > > > Repository: mesos > > > Description > ------- > > See [JIRA Ticket](https://issues.apache.org/jira/browse/MESOS-2348). > > > Diffs > ----- > > include/mesos/resources.hpp c242bcc29c490841354d6fdc8d0de16eeea602ed > src/common/resources.cpp 625922dd1abe2420736867c6043ca39be4e526ef > src/master/allocator/mesos/hierarchical.hpp > 2680d6231927867d5a8d75cbc42b81d6c75fc7f2 > src/master/master.hpp a466f9299f116e1c3aae6fc79c73c9c165383769 > src/master/validation.cpp cd1052adc877cf698f5f627eef6ff4008aa573d5 > src/slave/slave.cpp 0374ca061cb53dc7e3338366aa41b1c15fb3231c > src/tests/hierarchical_allocator_tests.cpp > eeecfb64540b16666915074aaffaa5d506b203bc > src/tests/resources_tests.cpp 3f98782fd437dba808d720bf8e9b94b8fa7e0feb > > Diff: https://reviews.apache.org/r/30911/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Michael Park > >
