mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Park" <mcyp...@gmail.com>
Subject Re: Review Request 37168: MESOS-3063 (Add an example framework using dynamic reservation)
Date Mon, 05 Oct 2015 23:12:41 GMT

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


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