----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37168/#review98746 -----------------------------------------------------------
Some preliminary comments. src/examples/dynamic_reservation_framework.cpp (line 40) <https://reviews.apache.org/r/37168/#comment155326> Remove newline. src/examples/dynamic_reservation_framework.cpp (line 49) <https://reviews.apache.org/r/37168/#comment155327> Not used. src/examples/dynamic_reservation_framework.cpp (lines 100 - 102) <https://reviews.apache.org/r/37168/#comment155335> Shouldn't we also make sure we only reserve `TASK_RESOURCES` amount of resources on only `totalTasks` number of agents? src/examples/dynamic_reservation_framework.cpp (lines 111 - 112) <https://reviews.apache.org/r/37168/#comment155317> It seems like we should stay in the `START` state until we've reserved `TASK_RESOURCES` amount of resources for this agent before transitioning to the `RESERVED` state, right? Looking at the `reserveResources` function, it doesn't even perform the `acceptOffers` if the offer resources doesn't contain sufficient resources. Shouldn't the `state = State::RESERVED` be dependent on the result of `reserveResources`? src/examples/dynamic_reservation_framework.cpp (line 180) <https://reviews.apache.org/r/37168/#comment155323> `s/sid/slaveId/` src/examples/dynamic_reservation_framework.cpp (line 183) <https://reviews.apache.org/r/37168/#comment155324> `s/executorID/executorId/` src/examples/dynamic_reservation_framework.cpp (line 184) <https://reviews.apache.org/r/37168/#comment155325> `s/slaveID/slaveId/` src/examples/dynamic_reservation_framework.cpp (line 228) <https://reviews.apache.org/r/37168/#comment155318> Why is this called `remaining`? I would think the "remaining" portion is `TASK_RESOURCES` - amount of already reserved resources? src/examples/dynamic_reservation_framework.cpp (line 229) <https://reviews.apache.org/r/37168/#comment155319> We should be careful here, as `find` does not do exact match. It can return resources with a non-matching role, for example. src/examples/dynamic_reservation_framework.cpp (line 242) <https://reviews.apache.org/r/37168/#comment155320> Why does this function perform side-effects? I think this function should look exactly the same as the one in `src/tests/mesos.hpp`. src/examples/dynamic_reservation_framework.cpp (line 252) <https://reviews.apache.org/r/37168/#comment155322> It doesn't makes sense to print this here as this function doesn't actually reserve any resources. src/examples/dynamic_reservation_framework.cpp (line 257) <https://reviews.apache.org/r/37168/#comment155321> Same comment as `RESERVE`. - Michael Park On Sept. 6, 2015, 4:11 a.m., Klaus Ma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37168/ > ----------------------------------------------------------- > > (Updated Sept. 6, 2015, 4:11 a.m.) > > > Review request for mesos and Michael Park. > > > Bugs: MESOS-3063 > https://issues.apache.org/jira/browse/MESOS-3063 > > > Repository: mesos > > > Description > ------- > > Provide example for dynamic reservation features. > > > Diffs > ----- > > src/Makefile.am 5fdca0f > src/examples/dynamic_reservation_framework.cpp PRE-CREATION > src/tests/dynamic_reservation_framework_test.sh PRE-CREATION > src/tests/examples_tests.cpp 3f56b30 > src/tests/flags.hpp 06da36d > src/tests/script.cpp bcc1fab > > Diff: https://reviews.apache.org/r/37168/diff/ > > > Testing > ------- > > make > make check > > > Thanks, > > Klaus Ma > >
