mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Klaus Ma" <klaus1982...@gmail.com>
Subject Re: Review Request 37168: MESOS-3063 (Add an example framework using dynamic reservation)
Date Mon, 09 Nov 2015 08:11:33 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message