----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28091/#review61656 -----------------------------------------------------------
Initial pass, curious if the other changes bundled in this review could be pulled out. include/mesos/resources.hpp <https://reviews.apache.org/r/28091/#comment103401> I'm curious, where is this relevant to the user of 'Resources'? Now that this TODO is implemented, does the user of 'Resources' need this NOTE? Maybe the important NOTE is that the caller *needs* to ensure that any externally specified resources are validated, right? include/mesos/resources.hpp <https://reviews.apache.org/r/28091/#comment103403> Is this comment still accurate? Looks like the empty cases are considered valid in the implementation below, which seems to make sense but does not match this comment. include/mesos/resources.hpp <https://reviews.apache.org/r/28091/#comment103399> Since you're changing the name of `isZero` to `zero`, how about `empty()` to be symmetric with resources.empty()? include/mesos/resources.hpp <https://reviews.apache.org/r/28091/#comment103412> Why is find() changed in this review? Is it related to flattening in some way? include/mesos/resources.hpp <https://reviews.apache.org/r/28091/#comment103406> I'm a bit confused at the utility of these, do you need to expose them in this change? Also, why return `Value::*`? Why not return C++ types, like double, std::set, and IntervalSet? include/mesos/resources.hpp <https://reviews.apache.org/r/28091/#comment103407> I wouldn't mention 'superset' now that it is called 'contains'. Seems it doesn't need a comment now that the signature is intuitive. src/common/resources.cpp <https://reviews.apache.org/r/28091/#comment103408> How about: `*this += resource`? src/common/resources.cpp <https://reviews.apache.org/r/28091/#comment103410> Why does this need to be prefixed with _? src/common/resources.cpp <https://reviews.apache.org/r/28091/#comment103409> How about: `*this += resource`? src/common/resources.cpp <https://reviews.apache.org/r/28091/#comment103411> Why the change here? src/tests/gc_tests.cpp <https://reviews.apache.org/r/28091/#comment103404> Looks like these tests should just be using resources.cpus() and resources.mem()? src/tests/resource_offers_tests.cpp <https://reviews.apache.org/r/28091/#comment103405> Ditto here and everywhere else w.r.t. cpus() and mem(). - Ben Mahler 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 > >
