> 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
> 
>

Reply via email to