You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Michael Park <mp...@apache.org> on 2016/04/04 03:40:21 UTC
Re: Review Request 37168: Add an example framework using dynamic
reservation.
-----------------------------------------------------------
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
>
>
Re: Review Request 37168: Add an example framework using dynamic
reservation.
Posted by Klaus Ma <kl...@gmail.com>.
> On April 4, 2016, 9:40 a.m., Michael Park wrote:
> > src/examples/dynamic_reservation_framework.cpp, lines 350-364
> > <https://reviews.apache.org/r/37168/diff/15/?file=1309849#file1309849line350>
> >
> > We should be able to just use the ones in `src/tests/mesos.hpp`.
I'd like to de-couple example from Mesos internal code, e.g. `tests/mesos.hpp`. Ideally, the user can build example with installed Mesos binaries & headers.
- Klaus
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37168/#review126769
-----------------------------------------------------------
On March 22, 2016, 11: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, 11: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
>
>
Re: Review Request 37168: Add an example framework using dynamic
reservation.
Posted by Klaus Ma <kl...@gmail.com>.
> On April 4, 2016, 9:40 a.m., Michael Park wrote:
> > src/examples/dynamic_reservation_framework.cpp, line 308
> > <https://reviews.apache.org/r/37168/diff/15/?file=1309849#file1309849line308>
> >
> > `static const Resources TASK_RESOURCES;`
>
> Klaus Ma wrote:
> If we mark it `const`, we have to move the following code into constructor's init, is that OK?
>
> ```
> Resources::parse(
> "cpus:" + stringify(CPUS_PER_TASK) +
> ";mem:" + stringify(MEM_PER_TASK)).get();
> ```
Fixed. My bad memory, static const member is ok to assign later.
- Klaus
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37168/#review126769
-----------------------------------------------------------
On April 5, 2016, 5:28 p.m., Klaus Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37168/
> -----------------------------------------------------------
>
> (Updated April 5, 2016, 5:28 p.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 f22ae5b3bd9336a56c802e0e51d39d6cb675caf2
> 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
>
>
Re: Review Request 37168: Add an example framework using dynamic
reservation.
Posted by Klaus Ma <kl...@gmail.com>.
> On April 4, 2016, 9:40 a.m., Michael Park wrote:
> > src/examples/dynamic_reservation_framework.cpp, lines 311-320
> > <https://reviews.apache.org/r/37168/diff/15/?file=1309849#file1309849line311>
> >
> > 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."
Thanks very much for your explaintation on `Try`; that's clear to me :).
BTW, `rc` means `return code`.
- Klaus
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37168/#review126769
-----------------------------------------------------------
On March 22, 2016, 11: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, 11: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
>
>
Re: Review Request 37168: Add an example framework using dynamic
reservation.
Posted by Michael Park <mp...@apache.org>.
> On April 4, 2016, 1:40 a.m., Michael Park wrote:
> > src/examples/dynamic_reservation_framework.cpp, lines 311-320
> > <https://reviews.apache.org/r/37168/diff/15/?file=1309849#file1309849line311>
> >
> > 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."
>
> Klaus Ma wrote:
> Thanks very much for your explaintation on `Try`; that's clear to me :).
>
> BTW, `rc` means `return code`.
I see. Thanks.
> On April 4, 2016, 1:40 a.m., Michael Park wrote:
> > src/examples/dynamic_reservation_framework.cpp, lines 350-364
> > <https://reviews.apache.org/r/37168/diff/15/?file=1309849#file1309849line350>
> >
> > We should be able to just use the ones in `src/tests/mesos.hpp`.
>
> Klaus Ma wrote:
> I'd like to de-couple example from Mesos internal code, e.g. `tests/mesos.hpp`. Ideally, the user can build example with installed Mesos binaries & headers.
I see. Ok, we can keep these then. Thanks!
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37168/#review126769
-----------------------------------------------------------
On April 5, 2016, 9:28 a.m., Klaus Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37168/
> -----------------------------------------------------------
>
> (Updated April 5, 2016, 9:28 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 f22ae5b3bd9336a56c802e0e51d39d6cb675caf2
> 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
>
>
Re: Review Request 37168: Add an example framework using dynamic
reservation.
Posted by Klaus Ma <kl...@gmail.com>.
> On April 4, 2016, 9:40 a.m., Michael Park wrote:
> > src/examples/dynamic_reservation_framework.cpp, lines 69-70
> > <https://reviews.apache.org/r/37168/diff/15/?file=1309849#file1309849line69>
> >
> > Initialize these in member init list.
> >
> > ```cpp
> > : ...
> > principal(_principal),
> > reservationInfo(createReservationInfo(principal)),
> > taskResources(TASK_RESOURCES(role, reservationInfo))
> > ```
`createReservationInfo` is also included in `test/mesos.hpp`, I'd like to avoid such dependency; so initialize it in constructor. I'd like to drop this comments, please feel free to re-open it if more comments :).
- Klaus
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37168/#review126769
-----------------------------------------------------------
On April 5, 2016, 5:28 p.m., Klaus Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37168/
> -----------------------------------------------------------
>
> (Updated April 5, 2016, 5:28 p.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 f22ae5b3bd9336a56c802e0e51d39d6cb675caf2
> 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
>
>
Re: Review Request 37168: Add an example framework using dynamic
reservation.
Posted by Klaus Ma <kl...@gmail.com>.
> On April 4, 2016, 9:40 a.m., Michael Park wrote:
> > src/examples/dynamic_reservation_framework.cpp, line 308
> > <https://reviews.apache.org/r/37168/diff/15/?file=1309849#file1309849line308>
> >
> > `static const Resources TASK_RESOURCES;`
If we mark it `const`, we have to move the following code into constructor's init, is that OK?
```
Resources::parse(
"cpus:" + stringify(CPUS_PER_TASK) +
";mem:" + stringify(MEM_PER_TASK)).get();
```
- Klaus
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37168/#review126769
-----------------------------------------------------------
On March 22, 2016, 11: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, 11: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
>
>