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