----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27555/#review59882 -----------------------------------------------------------
Can you split this review? It's really hard to follow all the changes in resources. You have 5 bullet points in your description. Maybe split according to those? Also, attach MESOS-1714 to it. include/mesos/resources.hpp <https://reviews.apache.org/r/27555/#comment101196> s/"resources"/these Resources/ include/mesos/resources.hpp <https://reviews.apache.org/r/27555/#comment101197> Not sure if "flatten" is the right word because the Resources are already stored as flat. include/mesos/resources.hpp <https://reviews.apache.org/r/27555/#comment101200> s/flattened// aren't Resources always flattened. s/"resources"/these Resources/ include/mesos/resources.hpp <https://reviews.apache.org/r/27555/#comment101201> what does "appropriate roles" mean? include/mesos/resources.hpp <https://reviews.apache.org/r/27555/#comment101199> s/targets/target/ ? include/mesos/resources.hpp <https://reviews.apache.org/r/27555/#comment101202> Are these across all roles? src/common/resources.cpp <https://reviews.apache.org/r/27555/#comment101356> why equals instead of the "==" operator? here and below. src/common/resources.cpp <https://reviews.apache.org/r/27555/#comment101357> s/object/objects/ src/common/resources.cpp <https://reviews.apache.org/r/27555/#comment101397> You probably want to explain what you mean by "named" Resource objects. src/common/resources.cpp <https://reviews.apache.org/r/27555/#comment101415> s/make/makes/ src/common/resources.cpp <https://reviews.apache.org/r/27555/#comment101421> Maybe include the resource name to help in debugging? src/common/resources.cpp <https://reviews.apache.org/r/27555/#comment101422> s/we/We/ src/tests/allocator_tests.cpp <https://reviews.apache.org/r/27555/#comment101406> no need for "flags.enabled_default_resources=false" here and below in this test? src/tests/resources_tests.cpp <https://reviews.apache.org/r/27555/#comment101412> Why is this killed? src/tests/resources_tests.cpp <https://reviews.apache.org/r/27555/#comment101411> Why not just use .cpus() and .mem(), here and everywhere else? - Vinod Kone On Nov. 4, 2014, 10:01 p.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27555/ > ----------------------------------------------------------- > > (Updated Nov. 4, 2014, 10:01 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 > ------- > > The purpose of the refactor is to support persistent disk resources. > > Here are the main things I've done in this refactor: > 1) Resource objects in Resources are stored in minimal format > (validated/non-zero). That allows us to kill isAllocatable, allocatable, > isZero, etc. > 2) 'matches' needs to be split into two pieces: one for combining and one for > removing, in order to support persitent disk resource. For example, one > cannot combine two Resource object with DiskInfo (it's like two disks), > however, you can do removal if they are identical. > 3) Some of the interfaces are not intuitive (e.g., <=, see details in the > ticket). I removed them in favor of more explicit interfaces. > 4) Unified all the validation code. > 5) Adjusted the tests accordingly. > > > Diffs > ----- > > include/mesos/resources.hpp 0e37170 > src/cli/execute.cpp ddaa20d > src/common/resources.cpp e9a0c85 > src/examples/low_level_scheduler_libprocess.cpp 7ef5ea7 > src/examples/low_level_scheduler_pthread.cpp 6e233a1 > src/examples/no_executor_framework.cpp f98a073 > src/examples/test_framework.cpp 187a611 > src/master/drf_sorter.cpp 5464900 > src/master/hierarchical_allocator_process.hpp 31dfb2c > src/master/http.cpp 3189933 > src/master/master.cpp d914786 > src/tests/allocator_tests.cpp 58e15aa > src/tests/gc_tests.cpp f7747e2 > src/tests/master_tests.cpp 2e52574 > src/tests/mesos.hpp c1d64a7 > src/tests/resource_offers_tests.cpp fe66432 > src/tests/resources_tests.cpp 3e50889 > src/tests/slave_recovery_tests.cpp 98e059f > src/tests/sorter_tests.cpp 0516ab5 > > Diff: https://reviews.apache.org/r/27555/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jie Yu > >
