----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28091/#review61673 -----------------------------------------------------------
include/mesos/resources.hpp <https://reviews.apache.org/r/28091/#comment103461> I think we can kill this? It doesn't look like this is currently being used, and validation happens when we're constructing `Resources` off of `google::protobuf::RepeatedPtrField<Resource>&` anyway. src/common/resources.cpp <https://reviews.apache.org/r/28091/#comment103435> +1 src/common/resources.cpp <https://reviews.apache.org/r/28091/#comment103434> In this case, it doesn't __need__ to be. But `Resources` has a member variable named `resources` which would be shadowed if this local variable had the same name. We should stay away from shadowing names as it's very error-prone and can be difficult to debug. src/common/resources.cpp <https://reviews.apache.org/r/28091/#comment103436> +1 src/common/resources.cpp <https://reviews.apache.org/r/28091/#comment103457> We should keep the `size` check as long as it's still valid since it's a trivial check that allows us to exit early. src/common/resources.cpp <https://reviews.apache.org/r/28091/#comment103451> How about a `find_if` then handle the cases? ```cpp auto iter = std::find_if( std::begin(resources), std::end(resources), lambda::bind(&combinable, that, lambda::_1)); if (iter != std::end(resources)) { *iter += that; } else { resources.Add()->CopyFrom(that); } ``` - Michael Park On Nov. 15, 2014, 7:18 a.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28091/ > ----------------------------------------------------------- > > (Updated Nov. 15, 2014, 7:18 a.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 0e37170 > src/common/resources.cpp e9a0c85 > src/examples/low_level_scheduler_libprocess.cpp 89b4318 > src/examples/low_level_scheduler_pthread.cpp e5cd48a > src/examples/test_framework.cpp e87198b > src/master/drf_sorter.cpp 5464900 > src/master/hierarchical_allocator_process.hpp 31dfb2c > src/master/http.cpp 3189933 > src/master/master.cpp 83c2f8a > src/slave/containerizer/containerizer.cpp f234835 > src/tests/gc_tests.cpp 8618ae1 > src/tests/mesos.hpp c1d64a7 > src/tests/resource_offers_tests.cpp 43820b0 > src/tests/resources_tests.cpp 3e50889 > > Diff: https://reviews.apache.org/r/28091/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jie Yu > >
