----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37168/#review101541 -----------------------------------------------------------
src/examples/dynamic_reservation_framework.cpp (lines 60 - 61) <https://reviews.apache.org/r/37168/#comment158964> `s/frist/first/` `s/un-reserved/unreserved/` src/examples/dynamic_reservation_framework.cpp (lines 120 - 123) <https://reviews.apache.org/r/37168/#comment158968> `reserveResources` returns an error when there are not enough resources available. While this is a necessary condition, it's not sufficient to assume that the reservation succeeded. We need to wait until the reserved resources are actually offered back to us before we can assume that the resources have been reserved. src/examples/dynamic_reservation_framework.cpp (lines 126 - 131) <https://reviews.apache.org/r/37168/#comment158978> Based on how this reads, it looks like we always launch tasks on the offer if the agent's state is `RESERVED`. In reality, we only launch tasks if the agent is in a reserved state and there has not been a task launched on this agent yet. I think we should capture that by having something like: ``` case State::RESERVED: if (/* no task have been launched on this agent yet && the offer has suffcient resources to launch a task */) { launchTask(driver, offer); } break; src/examples/dynamic_reservation_framework.cpp (lines 134 - 135) <https://reviews.apache.org/r/37168/#comment158979> Similar to above, we should only perform unreserve if the offer actually contains unreserved resources. src/examples/dynamic_reservation_framework.cpp (line 148) <https://reviews.apache.org/r/37168/#comment158967> `s/un-reserved/unreserved/` src/examples/dynamic_reservation_framework.cpp (lines 149 - 151) <https://reviews.apache.org/r/37168/#comment158966> Generally, having the same state be the initial state as well as the termination state and trying to differentiate between them is a bad idea. For example, we're assuming `states.empty()` is the termination state condition here but it's also the condition of the initial state. How do we know that we're not here because `offers` happened to be empty? Keeping the initial state and the termination state distinct will make the code easier to reason about and less error-prone. src/examples/dynamic_reservation_framework.cpp (lines 213 - 214) <https://reviews.apache.org/r/37168/#comment158974> This loop launches as many tasks as possible as long as they fit in the offered resources. I thought we only want to launch 1 task per agent and therefore 1 task per offer. I guess that behavior could be satisfied by making sure we only reserve 1 "task resources" worth of resources per agent, but we should capture that postcondition in here as a precondition. src/examples/dynamic_reservation_framework.cpp (lines 243 - 244) <https://reviews.apache.org/r/37168/#comment158971> It looks weird to see `resources_.flatten(role, reservationInfo)` but `TASK_RESOURCES.flatten(role)`, what's the reason for that? Also, `find` has funky behavior where it'll match resources that aren't exactly the thing that you specify to `find`. Is this funkiness accounted for? or do we just want to reserve `task_resources` if `resources.contains(task_resoures) == true`? src/examples/dynamic_reservation_framework.cpp (line 338) <https://reviews.apache.org/r/37168/#comment158981> This logic is typically inlined in all other example frameworks. Any particular reason as to why this is factored out to a function? and why is it all caps? src/tests/flags.hpp (lines 155 - 158) <https://reviews.apache.org/r/37168/#comment158980> I think we don't need this since we have: ``` os::setenv("MESOS_ROLES", flags.role.get()); ``` This seems to be how other example frameworks such as `persistent_volume_framework.cpp` have gotten away without this explicit `roles` flag. - Michael Park On Sept. 14, 2015, 1:48 p.m., Klaus Ma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37168/ > ----------------------------------------------------------- > > (Updated Sept. 14, 2015, 1:48 p.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 8963cea > 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 > >
