> On Nov. 18, 2014, 10:28 p.m., Ben Mahler wrote: > > src/common/resources.cpp, line 809 > > <https://reviews.apache.org/r/28091/diff/3/?file=766050#file766050line809> > > > > Why do we need the !empty guard? Seems like the += below should work > > even if it's empty..?
I would prefer to keep them. It's more explicit and avoid unnecessary loops if it's empty. > On Nov. 18, 2014, 10:28 p.m., Ben Mahler wrote: > > src/common/resources.cpp, line 870 > > <https://reviews.apache.org/r/28091/diff/3/?file=766050#file766050line870> > > > > Ditto above, why do you need the !empty check? Ditto. > On Nov. 18, 2014, 10:28 p.m., Ben Mahler wrote: > > include/mesos/resources.hpp, line 73 > > <https://reviews.apache.org/r/28091/diff/3/?file=766049#file766049line73> > > > > 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? Added the interface. Left a TODO. > On Nov. 18, 2014, 10:28 p.m., Ben Mahler wrote: > > src/common/resources.cpp, lines 428-487 > > <https://reviews.apache.org/r/28091/diff/3/?file=766050#file766050line428> > > > > 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 ;) Wow. No way I can say no to this comment:) - Jie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28091/#review61825 ----------------------------------------------------------- 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 > >
