> On Feb. 24, 2015, 3:08 a.m., Ben Mahler wrote: > > src/common/resources.cpp, lines 568-571 > > <https://reviews.apache.org/r/30911/diff/5/?file=873147#file873147line568> > > > > 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.
Reverted. I agree I should have indented the list. I'll remember to use (1) style list in the future. Personally I don't have a preference. > On Feb. 24, 2015, 3:08 a.m., Ben Mahler wrote: > > src/common/resources.cpp, lines 573-576 > > <https://reviews.apache.org/r/30911/diff/5/?file=873147#file873147line573> > > > > IMHO 'remaining' is what tells us that the approach used here is to > > pull out resources. Do you need to introduce this NOTE? Removed. I don't need to, I just thought maybe it would be helpful. > On Feb. 24, 2015, 3:08 a.m., Ben Mahler wrote: > > src/common/resources.cpp, lines 580-583 > > <https://reviews.apache.org/r/30911/diff/5/?file=873147#file873147line580> > > > > Should this be static? No, `operator()` cannot be static. > On Feb. 24, 2015, 3:08 a.m., Ben Mahler wrote: > > src/common/resources.cpp, line 591 > > <https://reviews.apache.org/r/30911/diff/5/?file=873147#file873147line591> > > > > Looks like a decent use case for '`auto`' given that the type of > > '`predicates`' is obvious and directly above this statement. Fixed. > On Feb. 24, 2015, 3:08 a.m., Ben Mahler wrote: > > src/master/allocator/mesos/hierarchical.hpp, lines 795-799 > > <https://reviews.apache.org/r/30911/diff/5/?file=873148#file873148line795> > > > > 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. This does work correctly, because of the way you introduced `reserved(role)` in https://reviews.apache.org/r/28985: ```cpp // Returns the reserved resources for the role. Note that the "*" // role represents unreserved resources, and will be ignored. Resources reserved(const std::string& role) const; ``` I could comment here explaining this, but if that's the case I wonder if the API isn't intuitive enough? I think part of the problem though is that we know that the concept of reservations are implemented through the state of `role`. Anyway, I'm open to suggestions here. > On Feb. 24, 2015, 3:08 a.m., Ben Mahler wrote: > > src/master/master.hpp, lines 926-939 > > <https://reviews.apache.org/r/30911/diff/5/?file=873149#file873149line926> > > > > 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. Left a TODO for now. > On Feb. 24, 2015, 3:08 a.m., Ben Mahler wrote: > > src/master/master.hpp, lines 928-931 > > <https://reviews.apache.org/r/30911/diff/5/?file=873149#file873149line928> > > > > No need for the code snippet (by the way your '`&`' is on the wrong > > side :)). Removed. (Oops, `clang-format` doesn't format comments...) > On Feb. 24, 2015, 3:08 a.m., Ben Mahler wrote: > > src/master/master.hpp, lines 933-939 > > <https://reviews.apache.org/r/30911/diff/5/?file=873149#file873149line933> > > > > should this be static? Ditto. `operator()` cannot be static. > On Feb. 24, 2015, 3:08 a.m., Ben Mahler wrote: > > src/master/validation.cpp, lines 103-104 > > <https://reviews.apache.org/r/30911/diff/5/?file=873150#file873150line103> > > > > ``` > > Resources volumes = Resources(resources).persistentVolumes(); > > > > foreach (const Resource& volume, volumes) { > > ... > > } > > ``` Fixed. > On Feb. 24, 2015, 3:08 a.m., Ben Mahler wrote: > > src/slave/slave.cpp, lines 1301-1302 > > <https://reviews.apache.org/r/30911/diff/5/?file=873151#file873151line1301> > > > > This is a bit hard to parse, can you do pull out a '`volumes`' variable > > (ditto from my comment above). Fixed. > On Feb. 24, 2015, 3:08 a.m., Ben Mahler wrote: > > src/slave/slave.cpp, lines 1311-1312 > > <https://reviews.apache.org/r/30911/diff/5/?file=873151#file873151line1311> > > > > Ditto here. Can you pull out a variable? Fixed. > On Feb. 24, 2015, 3:08 a.m., Ben Mahler wrote: > > src/slave/slave.cpp, lines 1985-1986 > > <https://reviews.apache.org/r/30911/diff/5/?file=873151#file873151line1985> > > > > Ditto here, mind pulling out a variable? Fixed. > On Feb. 24, 2015, 3:08 a.m., Ben Mahler wrote: > > src/tests/hierarchical_allocator_tests.cpp, lines 520-535 > > <https://reviews.apache.org/r/30911/diff/5/?file=873152#file873152line520> > > > > Why the changes to this file? Reverted. I must've had changes which put the lines over 80 chars and then reverted to a version that doesn't go over 80. - Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30911/#review73743 ----------------------------------------------------------- On Feb. 24, 2015, 7:48 a.m., Michael Park wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30911/ > ----------------------------------------------------------- > > (Updated Feb. 24, 2015, 7:48 a.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 a45bbaf696a6cc61a06daaa52a84f0014e7fe8cb > src/master/allocator/mesos/hierarchical.hpp > 9b7ded34ad7546f36dd41f64fe2e3cf41c1cf702 > src/master/master.hpp 8c44d6ed57ad1b94a17bef8142a5e6a15889a810 > src/master/validation.cpp e0936515d763a2cd3631a78062f66a304c0c0944 > src/slave/slave.cpp d44910838fc479b62200cdfd342ad13524504838 > src/tests/resources_tests.cpp 4298d108e32758599a28097028cc03b6bd7febfc > > Diff: https://reviews.apache.org/r/30911/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Michael Park > >
