mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michael Park <mp...@apache.org>
Subject Re: Review Request 37168: Add an example framework using dynamic reservation.
Date Mon, 04 Apr 2016 01:40:21 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37168/#review126769
-----------------------------------------------------------




src/examples/dynamic_reservation_framework.cpp (line 52)
<https://reviews.apache.org/r/37168/#comment189833>

    `s/slave/agent/`
    `s/when offered to framework/when they are offered to the framework/`



src/examples/dynamic_reservation_framework.cpp (lines 69 - 70)
<https://reviews.apache.org/r/37168/#comment189829>

    Initialize these in member init list.
    
    ```cpp
      : ...
        principal(_principal),
        reservationInfo(createReservationInfo(principal)),
        taskResources(TASK_RESOURCES(role, reservationInfo))
    ```



src/examples/dynamic_reservation_framework.cpp (lines 90 - 91)
<https://reviews.apache.org/r/37168/#comment189832>

    ```cpp
    LOG(INFO) << "Received offer " << offer.id() << " with "
              << offer.resources();
    ```



src/examples/dynamic_reservation_framework.cpp (line 93)
<https://reviews.apache.org/r/37168/#comment189834>

    `s/If framework/If the framework/`



src/examples/dynamic_reservation_framework.cpp (line 94)
<https://reviews.apache.org/r/37168/#comment189835>

    Replace quotes around `State::INIT` with backticks.



src/examples/dynamic_reservation_framework.cpp (line 97)
<https://reviews.apache.org/r/37168/#comment189836>

    `s/did not/do not/`
    `s/waiting/wait/`



src/examples/dynamic_reservation_framework.cpp (line 106)
<https://reviews.apache.org/r/37168/#comment189837>

    `const State`



src/examples/dynamic_reservation_framework.cpp (lines 109 - 117)
<https://reviews.apache.org/r/37168/#comment189838>

    We format cases with scopes like this:
    ```cpp
            case State::INIT: {
              /* ... */
              break;
            }
    ```
    
    Here and below.



src/examples/dynamic_reservation_framework.cpp (lines 114 - 115)
<https://reviews.apache.org/r/37168/#comment189840>

    I think we can get rid of `reserveResources` and `updateState` and just do the following.
See below for I don't think these are useful abstractions.
    ```cpp
        const Resources& resources = offer.resources();
        Resources unreserved = resources.unreserved();
    
        if (!unreserved.contains(TASK_RESOURCES)) {
          LOG(INFO) << "Insufficient resources for task in offer " + stringify(offer.id());
          break;
        }
        
        driver->acceptOffers({offer.id()}, {RESERVE(taskResources)});
        states[offer.slave_id()] = State::RESERVING;
        break;
    ```



src/examples/dynamic_reservation_framework.cpp (lines 121 - 144)
<https://reviews.apache.org/r/37168/#comment189843>

    How about the following control flow?
    
    ```cpp
    const Resources& resources = offer.resources();
    
    ...
    
    case State::RESERVING: {
      Resources reserved = resources.reserved(role);
      if (reserved.contains(taskResources)) {
        states[offer.slave_id()] = State::RESERVED;
      }
      // We fallthorugh here to save an offer cycle.
      // [[fallthrough]]
    }
    case State::RESERVED: {
      Resources reserved = resources.reserved(role);
      
      if (tasksLaunched == totalTasks) {
        /* unreserve resources */
        break;
      }
    
      CHECK(reserved.contains(taskResources));
      CHECK(tasksLaunched < totalTasks);
      /* launch tasks */
      break;
    }
    ```



src/examples/dynamic_reservation_framework.cpp (lines 146 - 147)
<https://reviews.apache.org/r/37168/#comment189844>

    ```cpp
    LOG(INFO) << "The task on " << offer.slave_id()
              << " is running, waiting for task done";
    ```



src/examples/dynamic_reservation_framework.cpp (line 154)
<https://reviews.apache.org/r/37168/#comment189845>

    ```cpp
    states[offer.slave_id()] = State::UNRESERVED;
    ```



src/examples/dynamic_reservation_framework.cpp (line 187)
<https://reviews.apache.org/r/37168/#comment189846>

    `++tasksFinished;`



src/examples/dynamic_reservation_framework.cpp (line 189)
<https://reviews.apache.org/r/37168/#comment189847>

    ```cpp
    CHECK(states[status.slave_id()] == State::TASK_RUNNING);
    states[status.slave_id()] = State::RESERVED;
    ```



src/examples/dynamic_reservation_framework.cpp (lines 191 - 192)
<https://reviews.apache.org/r/37168/#comment189848>

    ```cpp
    LOG(INFO) << "Task " << taskId << " is finished at slave "
              << status.slave_id();
    ```



src/examples/dynamic_reservation_framework.cpp (lines 200 - 204)
<https://reviews.apache.org/r/37168/#comment189849>

    ```cpp
    LOG(INFO) << "Aborting because task " << taskId
              << " is in unexpected state " << status.state()
              << " with reason " << status.reason()
              << " from source " << status.source()
              << " with message '" << status.message() << "'";
    ```



src/examples/dynamic_reservation_framework.cpp (lines 230 - 258)
<https://reviews.apache.org/r/37168/#comment189851>

    Let's just inline this function as I've mentioned for `reserveResources` and `unreserveResources`.



src/examples/dynamic_reservation_framework.cpp (lines 274 - 278)
<https://reviews.apache.org/r/37168/#comment189852>

    With my suggestion above, we would always go from `RESERVING` to `RESERVED`. The `RESERVING`
-> `UNRESERVING` case will simply go through the `RESERVED` state: `RESERVING` -> `RESERVED`
-> `UNRESERVING`.



src/examples/dynamic_reservation_framework.cpp (line 308)
<https://reviews.apache.org/r/37168/#comment189830>

    `static const Resources TASK_RESOURCES;`



src/examples/dynamic_reservation_framework.cpp (lines 311 - 320)
<https://reviews.apache.org/r/37168/#comment189839>

    Conceptutally, `Try` is not something we pass around like this. A function returning a
`Try` is emulating a function that throws an exception. So if a `Try` results in an error,
we need to handle it immediately, or propagate the error. We do not pass it along to a subsequent
function.
    
    There are other issues here as well. Even if we were to pass it around, it should have
been passed by `const &`. What does `rc` stand for?
    
    Functionally speaking, this says that if `rc` is an error, we log saying we failed to
update the state of an agent. But `reserveResources` returns `Error` if the offer doesn't
contain sufficient amount of resources. This has really little to do with "failing to update
the state of an agent."



src/examples/dynamic_reservation_framework.cpp (lines 322 - 334)
<https://reviews.apache.org/r/37168/#comment189841>

    I don't think this is a useful abstraction because (1) it doesn't do much and (2) it's
only used in 1 place.



src/examples/dynamic_reservation_framework.cpp (lines 336 - 348)
<https://reviews.apache.org/r/37168/#comment189853>

    Same as `reserveResources`. Let's just inline it.



src/examples/dynamic_reservation_framework.cpp (lines 350 - 364)
<https://reviews.apache.org/r/37168/#comment189831>

    We should be able to just use the ones in `src/tests/mesos.hpp`.


- Michael Park


On March 22, 2016, 3:44 a.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37168/
> -----------------------------------------------------------
> 
> (Updated March 22, 2016, 3:44 a.m.)
> 
> 
> Review request for mesos, Greg Mann, Joerg Schad, 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 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
>   src/examples/dynamic_reservation_framework.cpp PRE-CREATION 
>   src/tests/dynamic_reservation_framework_test.sh PRE-CREATION 
>   src/tests/examples_tests.cpp 6ddac17bb2ac0330bcc09eaab975ae70e84a7695 
> 
> 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