> On Oct. 6, 2015, 7:12 a.m., Michael Park wrote: > > src/examples/dynamic_reservation_framework.cpp, lines 120-123 > > <https://reviews.apache.org/r/37168/diff/5/?file=1072547#file1072547line120> > > > > `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.
I used to add `RESERVING` state to show the case that the framework did not get the reserved resources; but it seems not necessary: the `RESERVING` state will transfer to `RESERVED` and dispatch task, the code logic of `RESERVING` & `RESERVED` are same. Should we add `RESERVING` state to make it more readable or change the term to `RESERVE`? > On Oct. 6, 2015, 7:12 a.m., Michael Park wrote: > > src/examples/dynamic_reservation_framework.cpp, lines 126-131 > > <https://reviews.apache.org/r/37168/diff/5/?file=1072547#file1072547line126> > > > > 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; Agree; for the condition `no task have been launched on this agent yet`, it's guantee by state of slave; and also check whether reserved resources contains task resource. > On Oct. 6, 2015, 7:12 a.m., Michael Park wrote: > > src/examples/dynamic_reservation_framework.cpp, lines 149-151 > > <https://reviews.apache.org/r/37168/diff/5/?file=1072547#file1072547line149> > > > > 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. Address by adding `tasksFinished == totalTasks`: when all tasks are done & states is empty, stop the driver. > On Oct. 6, 2015, 7:12 a.m., Michael Park wrote: > > src/examples/dynamic_reservation_framework.cpp, lines 243-244 > > <https://reviews.apache.org/r/37168/diff/5/?file=1072547#file1072547line243> > > > > 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`? Update to check `resources.contains(task_resoures)`. > On Oct. 6, 2015, 7:12 a.m., Michael Park wrote: > > src/examples/dynamic_reservation_framework.cpp, lines 213-214 > > <https://reviews.apache.org/r/37168/diff/5/?file=1072547#file1072547line213> > > > > 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. Update to only dispatch 1 task to the reserved resources; and use `reserved.contains(taskResources)` to check whether got reserved resources by this framework. > On Oct. 6, 2015, 7:12 a.m., Michael Park wrote: > > src/examples/dynamic_reservation_framework.cpp, line 338 > > <https://reviews.apache.org/r/37168/diff/5/?file=1072547#file1072547line338> > > > > 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? OK :). Moved to `main()` > On Oct. 6, 2015, 7:12 a.m., Michael Park wrote: > > src/tests/flags.hpp, lines 155-158 > > <https://reviews.apache.org/r/37168/diff/5/?file=1072550#file1072550line155> > > > > 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. Agree :). Addressed in new patch. - Klaus ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37168/#review101541 ----------------------------------------------------------- On Nov. 9, 2015, 4:10 p.m., Klaus Ma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37168/ > ----------------------------------------------------------- > > (Updated Nov. 9, 2015, 4:10 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 c479aca > src/examples/dynamic_reservation_framework.cpp PRE-CREATION > src/tests/dynamic_reservation_framework_test.sh PRE-CREATION > src/tests/examples_tests.cpp 3f56b30 > > Diff: https://reviews.apache.org/r/37168/diff/ > > > Testing > ------- > > make > make check > > > Thanks, > > Klaus Ma > >
