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

Reply via email to