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 2017/06/12 18:46:58 UTC
Review Request 60021: Corrected the use of `toUnreserved()` to be more
accurate.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60021/
-----------------------------------------------------------
Review request for mesos and Benjamin Mahler.
Bugs: MESOS-7655
https://issues.apache.org/jira/browse/MESOS-7655
Repository: mesos
Description
-------
The simple change from `flatten()` to `toUnreserved()` needs to be updated to account for reservation refinement.
When we look for resources to be reserved (for example, reserving 4 cpus for role `R`"), it used to be sufficient
to simply look for unreserved versions of the resources (4 cpus, unreserved). Now, we need to look for version
of the resources with the last reservation popped. Note that for the single reservation case, the behavior
remains unchanged.
Diffs
-----
src/common/resources.cpp f89f1aae7845f1e93dd5947de2e7a8e2bfce8bc4
src/master/http.cpp 1dcfe6ef00b0e3984deb79a511e665f638661323
src/v1/resources.cpp 24240eedc4b2747ca02998534437318c3396db4d
Diff: https://reviews.apache.org/r/60021/diff/1/
Testing
-------
Thanks,
Michael Park
Re: Review Request 60021: Adjusted (UN)RESERVE operations to handle
reservation refinement [8/N].
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60021/
-----------------------------------------------------------
(Updated June 19, 2017, 10:33 p.m.)
Review request for mesos and Benjamin Mahler.
Bugs: MESOS-7655
https://issues.apache.org/jira/browse/MESOS-7655
Repository: mesos
Description (updated)
-------
The logic for applying RESERVE operations now needs to ensure that the
pre-reserved resources are being refined. In other words, the RESERVE
operation should be "pushing" a single reservation to the stack. Note
that we only support "pushing" a single reservation at a time.
For UNRESERVE, the operation should "pop" the last reservation off the
stack. We only support "popping" a single reservation at a time.
This patch also replaces all of the uses of `flatten()` to
`toUnreserved()` and also removes `flatten()` entirely. We also replace
the uses of `flatten(role, reservation)` except in tests. We'll remove
this version in later patch.
Diffs (updated)
-----
include/mesos/resources.hpp be2e377ab9a7332adf373c585e5fafd47ce79f7e
include/mesos/v1/resources.hpp fe62166f20088504bdf90a178d3e3b0a94cdc1ac
src/cli/execute.cpp 11a2569f788a2ee1ceaf13ab0d40d1d1b275f27a
src/common/resources.cpp ef54678d02c03460dcc01779bf5cd974f486f054
src/examples/balloon_framework.cpp bfaae3796afb426459ef5cff3a37634a8242032e
src/examples/disk_full_framework.cpp a73e6cf285b9a74492d476a624915235e079051f
src/examples/long_lived_framework.cpp bbe8606c8d2572bbbbad695e44fa23ce7b6129eb
src/examples/test_framework.cpp 05ddc89d953d9b968b78f98fec819aeda7f79e26
src/examples/test_http_framework.cpp 471835c349e0da031a540ed48881227a25887ba7
src/master/allocator/mesos/hierarchical.cpp 775fe4115d5f55be1e8542fa4f94699396d9e2a1
src/master/http.cpp 4f838493104146e1a6b68eb64d8ec122cd54b7f7
src/tests/mesos.hpp b93833394f9fe2626f5b8e9d84bb5d336f1eee0b
src/tests/reservation_endpoints_tests.cpp ba283ee74fe27998a01c43e1ab6a8ce0089181fa
src/tests/resources_tests.cpp a44490a4f4fbd3af3b95c9e9a167a96c6e0f40f9
src/v1/resources.cpp c1b6b8a4e5aaa7a45aa59f1808432b831a1c72a6
Diff: https://reviews.apache.org/r/60021/diff/7/
Changes: https://reviews.apache.org/r/60021/diff/6-7/
Testing
-------
Thanks,
Michael Park
Re: Review Request 60021: Updated the use of `toUnreserved()`.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60021/#review177851
-----------------------------------------------------------
Fix it, then Ship it!
Looks good, although this merits a description, how about:
```
Updated (UN)RESERVE operation application to handle reservation refinement.
The logic for applying RESERVE operations now needs to ensure that the
pre-reserved resources are being refined. In other words, the RESERVE
operation should be "pushing" a single reservation to the stack. Note
that we only support "pushing" a single reservation at a time.
For UNRESERVE, the operation should "pop" the last reservation off the
stack. We only support "popping" a single reservation at a time.
```
src/common/resources.cpp
Line 1474 (original), 1474 (patched)
<https://reviews.apache.org/r/60021/#comment251565>
Can you add a note here saying we only allow "pushing" a single reservation at a time to the "stack" of reservations?
I noted we should document this under [MESOS-7663](https://issues.apache.org/jira/browse/MESOS-7663).
src/common/resources.cpp
Line 1505 (original), 1505 (patched)
<https://reviews.apache.org/r/60021/#comment251566>
Ditto here, can you add a note here saying we only allow "popping" a single reservation at a time to the "stack" of reservations?
src/master/http.cpp
Lines 2245-2248 (original), 2245-2248 (patched)
<https://reviews.apache.org/r/60021/#comment251564>
Do you want to re-write this note to reflect that we only allow the operator to push one reservation at a time?
Also, not yours, but reading this code I'm left confused as to why we need to pass the popped resources of the operation and the operation into the continuation. I think the following would clear both of these up?
```
// We only allow pushing a single reservation at a time, so
// we require the resources with one reservation "popped" to
// be present on the agent.
Resources required = resources.popReservation();
return _operation(slaveId, required, operation);
```
src/master/http.cpp
Lines 2245-2249 (original), 2245-2250 (patched)
<https://reviews.apache.org/r/60021/#comment251638>
Shouldn't I see a similar change in `Master::Http::_unreserve` logic? Striked me as a little odd that they both didn't need a change.
src/master/http.cpp
Lines 2250 (patched)
<https://reviews.apache.org/r/60021/#comment251637>
Why did you need to wrap with Resources() here? Did you originally have popReservation as a mutating fucntion and this is a leftover?
- Benjamin Mahler
On June 13, 2017, 8:37 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60021/
> -----------------------------------------------------------
>
> (Updated June 13, 2017, 8:37 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-7655
> https://issues.apache.org/jira/browse/MESOS-7655
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated the use of `toUnreserved()`.
>
>
> Diffs
> -----
>
> src/common/resources.cpp 7b3fae1a34150964ba3831a3aef2f868e338aef8
> src/master/http.cpp 1dcfe6ef00b0e3984deb79a511e665f638661323
> src/v1/resources.cpp 236fe17730918479a33314e51d14f3cc1679d432
>
>
> Diff: https://reviews.apache.org/r/60021/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 60021: Corrected the use of `toUnreserved()` to be
more accurate.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60021/
-----------------------------------------------------------
(Updated June 13, 2017, 1:37 a.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Rebased.
Bugs: MESOS-7655
https://issues.apache.org/jira/browse/MESOS-7655
Repository: mesos
Description (updated)
-------
Corrected the use of `toUnreserved()` to be more accurate.
Diffs (updated)
-----
src/common/resources.cpp f89f1aae7845f1e93dd5947de2e7a8e2bfce8bc4
src/master/http.cpp 1dcfe6ef00b0e3984deb79a511e665f638661323
src/v1/resources.cpp 24240eedc4b2747ca02998534437318c3396db4d
Diff: https://reviews.apache.org/r/60021/diff/2/
Changes: https://reviews.apache.org/r/60021/diff/1-2/
Testing
-------
Thanks,
Michael Park