----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28091/#review61825 -----------------------------------------------------------
Ship it! Modulo comments. Looks like this could have been broken apart a bit more, next time! ;) include/mesos/resources.hpp <https://reviews.apache.org/r/28091/#comment103757> I thought you hid the arithmetic operators for 'Resource', did you mean 'Resources' here? How about adjusting the last sentence: // It is the caller's responsibility to validate any Resource or repeated Resource protobufs before constructing a Resources object. Otherwise, invalid resources will be silently stripped. include/mesos/resources.hpp <https://reviews.apache.org/r/28091/#comment103758> "valid.", whoops? Seems a bit odd to only call out what it means for a scalar to be invalid, but to omit set/range invalidity (e.g. begin > end). Maybe best to just delete this last sentence? include/mesos/resources.hpp <https://reviews.apache.org/r/28091/#comment103760> Looking at the master code, it seems like we need a validate that operates across individual resources: ``` static Option<Error> validate(const RepeatedPtrField<Resource>&); ``` Because currently we assume that if each resource is valid, the collection of resources is also valid, which is not true (e.g. multiple usage of item in a set, of a port, of DiskInfo in the future). Thoughts? include/mesos/resources.hpp <https://reviews.apache.org/r/28091/#comment103763> Just so we're in agreement, this is also "allocation" logic, although it's trying to be generic by allowing the caller to specify role preference.. But does it even work for port allocation? Generally frameworks will want _any_ port, as opposed to a _specific_ port. I'm kind of inclined to pull out resource utilities for our example frameworks to leverage, as opposed to placing this inside 'Resources'. How about for now, we add the following TODO: // TODO(jieyu): Find contains some allocation logic for scalars and fixed set / range elements. However, this is not sufficient for schedulers that want, say, any N available ports. We should consider moving this to an internal "allocation" library for our example frameworks to leverage. Sound good? include/mesos/resources.hpp <https://reviews.apache.org/r/28091/#comment103762> Why the change here? You can keep `get()` templatized, even if you want to remove the default argument: ``` template <typename T> T get(const std::string& name); // Caller. Value::Set s = r.get<Value::Set>("items"); ``` Right? src/common/resources.cpp <https://reviews.apache.org/r/28091/#comment103767> How about: "Invalid scalar resource: value < 0" src/common/resources.cpp <https://reviews.apache.org/r/28091/#comment103768> "Invalid ranges resource: begin > end" src/common/resources.cpp <https://reviews.apache.org/r/28091/#comment103769> "Invalid ranges resource: overlapping ranges" src/common/resources.cpp <https://reviews.apache.org/r/28091/#comment103770> "Invalid set resource: duplicate elements" src/common/resources.cpp <https://reviews.apache.org/r/28091/#comment103771> Do you need this comment? :) src/common/resources.cpp <https://reviews.apache.org/r/28091/#comment103919> Can you create an overload that takes a '`Resource`'? Then we can leverage that to break this apart a bit: ``` Option<Resources> Resources::find(const Resources& targets) const { Resources total; foreach (const Resource& target, targets) { Option<Resource> found = find(target); // XXX Use overload. // Each target needs to be found! if (found.isNone()) { return None(); } total += found.get(); } return total; } ``` Then I think the other `find(Resource)` gets a bit simpler with a hidden abstraction for role filtering: ``` struct RoleFilter { RoleFilter() : type(ANY) {} /*implicit*/ RoleFilter(string value) : type(SOME), value(_value) {} static RoleFilter any() { return RoleFilter(); } enum { ANY, SOME } type; std::string value; }; // The target resource may span multiple roles, so this returns Resources. Option<Resources> Resources::find(const Resource& target) const { Resources total = *this; Resources remaining = Resources(target).flatten(); Resources found; // First look in the target role, then "*", then any remaining role. vector<RoleFilter> filters = { target.role(), "*", Role::any() }; foreach (const RoleFilter& filter, filters) { foreach (const Resource& resource, total.filter(role)) { // Need to flatten to ignore the roles in contains(). Rescources flattened = filtered.flatten(); if (flattened.contains(remaining)) { // Done! return found + resource; } else if (remaining.contains(flattened)) { found += filtered; remaining -= resource; total -= resource; } } } return None(); } // Hide this for now! Resources filter(const RoleFilter& filter) const { if (filter.type == ANY) { return *this; } Resources filtered; foreach (const Resource& resource, resources) { if (resource.role() == filter.role()) { filtered += resource; } } return filtered; } ``` We could make the other `find` hidden if you think callers don't want it, `filter()` and the RoleFilter can be private for now. Feel free to clean and simplify my code further], I didn't compile it ;) src/common/resources.cpp <https://reviews.apache.org/r/28091/#comment103944> Do you need mesos::? Reads a bit strangely. src/common/resources.cpp <https://reviews.apache.org/r/28091/#comment103945> Why do we need the !empty guard? Seems like the += below should work even if it's empty..? src/common/resources.cpp <https://reviews.apache.org/r/28091/#comment103947> Ditto above, why do you need the !empty check? src/common/resources.cpp <https://reviews.apache.org/r/28091/#comment103954> For what it's worth, this could look very similar to your += code, might be nice to keep them consistent? ``` Resources& Resources::operator -= (const Resource& that) { if (validate(that).isNone()) { auto it = find_if( resources.begin(), resources.end(), lambda::bind(&removable, lambda::_1, that)); if (it != resources.end()) { *it -= resource; if (empty(*it)) { resources.DeleteSubrange(it - resources.begin(), 1); } } } return *this; } ``` You could use std::distance if the iterator subtraction isn't supported, but it looks like it is. That, or we should use the existing looping construct in both of them, to keep them consistent. src/common/resources.cpp <https://reviews.apache.org/r/28091/#comment103948> Let's just make this a pointer, in line with most of our code :) src/common/resources.cpp <https://reviews.apache.org/r/28091/#comment103958> Why do you need to validate here?? Definitely needs a comment, but I assume it should not be possible? src/master/master.cpp <https://reviews.apache.org/r/28091/#comment103750> I think it makes more sense to provide validation at the resources level (`validate(RepeatedPtrField<Resource>)`), so that you can do: Option<Error> error = Resources::validate(task.resources); This lets us take care of validations that we're currently missing. For example, the task used a set resource twice in different `Resource` object, or the task has the same port specified in multiple `Resource` objects, and in the future, the same will apply for DiskInfo. src/master/master.cpp <https://reviews.apache.org/r/28091/#comment103740> Looks like this TODO can be removed. src/slave/containerizer/containerizer.cpp <https://reviews.apache.org/r/28091/#comment103739> No comment...? :) How about a block comment after we parse the resources? // NOTE: We need to check for the "cpus" string within the flag // because once Resources are parsed, we cannot distinguish between // (1) "cpus:0", and // (2) no cpus specified. // We only auto-detect cpus in case (2). // The same logic applies for the other resources! - Ben Mahler On Nov. 17, 2014, 8:35 p.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28091/ > ----------------------------------------------------------- > > (Updated Nov. 17, 2014, 8:35 p.m.) > > > Review request for mesos, Ben Mahler and Vinod Kone. > > > Bugs: MESOS-1974 > https://issues.apache.org/jira/browse/MESOS-1974 > > > Repository: mesos-git > > > Description > ------- > > See summary. Always combine Resource objects in Resources and disallow > invalid/zero Resource objects. > > > Diffs > ----- > > include/mesos/resources.hpp 0e37170262d3470570a3436b7835bb1d4a121056 > src/common/resources.cpp e9a0c85dc3748aa635843d98cd5993d5648ec5c2 > src/examples/low_level_scheduler_libprocess.cpp > 89b43181c9ea012e04018482fb9edd2f8091d63e > src/examples/low_level_scheduler_pthread.cpp > e5cd48a76e201174af1e3a5f45576a342738dceb > src/examples/test_framework.cpp e87198bba7c60005d01e6fd58965ac322a3a53e3 > src/master/drf_sorter.cpp 54649003745721e75e715b9d2e950e1b38f6b9db > src/master/hierarchical_allocator_process.hpp > 31dfb2cc86e2e74da43f05fbada10976ce65f3e4 > src/master/http.cpp 31899334b905f83a2305c51c4bfefbee623e697e > src/master/master.cpp 83c2f8a94c00a1b07c5e6ab4e10404e0b3fdf2da > src/slave/containerizer/containerizer.cpp > f234835180b42331f731d92cd2ad7bd56b6efa70 > src/tests/gc_tests.cpp 8618ae12b337bea19a82dec52455cd19cc735d89 > src/tests/mesos.hpp c1d64a73ff2312a10d4e809072d219e60c28a76f > src/tests/resource_offers_tests.cpp > 43820b0a709b6e8643940b70183e277000d4ba35 > src/tests/resources_tests.cpp 3e50889ff2433930bd137a9d836d0a8bfc2d0388 > > Diff: https://reviews.apache.org/r/28091/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jie Yu > >
