You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2017/12/09 02:01:25 UTC
Review Request 64477: Refactored agent to keep track of local resource
providers.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64477/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier, Greg Mann, and Jan Schlicht.
Repository: mesos
Description
-------
Currently, we don't explicitly keep track of local resources providers.
This causes the logic for a few methods to be quite complex because we
need to reconstruct the resource provider information everytime.
Diffs
-----
src/resource_provider/manager.cpp f98611c351f11b15281dd3ef673ef80db1c44f07
src/resource_provider/message.hpp bbf6bb2a0a4aa901ac038cdbe4dd46acc0f2e453
src/slave/http.cpp 3de874cfd749180888b7b3988871d4d34bff35f8
src/slave/slave.hpp b3a1e70169ed1abbfc815821771715197a63f2df
src/slave/slave.cpp 373e393ca1e7c0c30c3474cc9e630e25ad92f235
src/tests/resource_provider_manager_tests.cpp a6eb4c9a303780029244e069bdf550a8cd9c7bb4
Diff: https://reviews.apache.org/r/64477/diff/1/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 64477: Refactored agent to keep track of local
resource providers.
Posted by Jie Yu <yu...@gmail.com>.
> On Dec. 11, 2017, 11:24 a.m., Benjamin Bannier wrote:
> > src/slave/slave.cpp
> > Lines 7294 (patched)
> > <https://reviews.apache.org/r/64477/diff/2/?file=1912141#file1912141line7387>
> >
> > Since it is possible to e.g., `RESERVE` an empty `Resources`, I believe we could currently trigger this `CHECK`.
> >
> > Even if we added master validation in the future it is still tricky to decide what status update manager should keep track of reliably sending the update. I think it would make sense to bookkeep this operation in a failed state to the agent so its status update manager can in the future deliver the offer operation status to the framework.
> >
> > For now we could just drop operations without an extractable resource provider id here with some logging and a `TODO`.
I prefer to use a CHECK here. I'll add a TODO to fix the master validation.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64477/#review193380
-----------------------------------------------------------
On Dec. 10, 2017, 5:11 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64477/
> -----------------------------------------------------------
>
> (Updated Dec. 10, 2017, 5:11 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Greg Mann, and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Currently, we don't explicitly keep track of local resources providers.
> This causes the logic for a few methods to be quite complex because we
> need to reconstruct the resource provider information everytime.
>
>
> Diffs
> -----
>
> src/resource_provider/manager.cpp f98611c351f11b15281dd3ef673ef80db1c44f07
> src/resource_provider/message.hpp bbf6bb2a0a4aa901ac038cdbe4dd46acc0f2e453
> src/slave/http.cpp 738786fc4b85903f187cf0988a3fca488ea2767d
> src/slave/slave.hpp b3a1e70169ed1abbfc815821771715197a63f2df
> src/slave/slave.cpp 373e393ca1e7c0c30c3474cc9e630e25ad92f235
> src/tests/resource_provider_manager_tests.cpp a6eb4c9a303780029244e069bdf550a8cd9c7bb4
>
>
> Diff: https://reviews.apache.org/r/64477/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 64477: Refactored agent to keep track of local
resource providers.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64477/#review193380
-----------------------------------------------------------
src/resource_provider/manager.cpp
Lines 656-660 (original), 656-667 (patched)
<https://reviews.apache.org/r/64477/#comment271927>
Nit: I'd prefer if we'd prepare a `hashmap` of operations before creating `updateState` and `move` it into an arg in the aggregate initialization. That way we do not construct a half-valid `updateState`.
hashmap<UUID, OfferOperation> offerOperations;
foreach (const OfferOperation &operation, update.operations()) {
Try<UUID> uuid = UUID::fromBytes(operation.operation_uuid());
CHECK_SOME(uuid);
offerOperations.put(uuid.get(), operation);
}
ResourceProviderMessage::UpdateState updateState{
resourceProvider->info,
resourceVersion.get(),
update.resources(),
std::move(offerOperations)};
src/slave/slave.hpp
Lines 1072 (patched)
<https://reviews.apache.org/r/64477/#comment271931>
We do not need this backpointer, so let's please remove it (IMO it is also not very good design since it introduces too much coupling).
src/slave/slave.cpp
Lines 1548 (patched)
<https://reviews.apache.org/r/64477/#comment271923>
Let's add a `CHECK` here that `info.has_id()`.
src/slave/slave.cpp
Lines 1569 (patched)
<https://reviews.apache.org/r/64477/#comment271924>
Let's add a `CHECK` here that `info.has_id()`.
src/slave/slave.cpp
Line 2285 (original), 2299 (patched)
<https://reviews.apache.org/r/64477/#comment271929>
This piece of code seems fragile under future changes even before this change since it assumed that the agent _always_ knows as many resource providers as the master. I wonder if we should explicitly allow for the agent to not know about a `resourceProvider` here instead of crashing, e.g., remove the `CHECK_NOTNULL` but instead
if (!resourceProvider ||
resourceProvider->resourceVerdsion !=
receivedResourceVersions.at(resourceProviderId.get())) {
src/slave/slave.cpp
Lines 7017-7019 (original)
<https://reviews.apache.org/r/64477/#comment271930>
This information seemed like an important implementation detail. Why are we removing this comment even though the approach has not changed?
src/slave/slave.cpp
Lines 7133 (patched)
<https://reviews.apache.org/r/64477/#comment271932>
Not yours, but we should add
CHECK(totalResources.contains(resourceProvider->totalResources));
src/slave/slave.cpp
Lines 7294 (patched)
<https://reviews.apache.org/r/64477/#comment271935>
Since it is possible to e.g., `RESERVE` an empty `Resources`, I believe we could currently trigger this `CHECK`.
Even if we added master validation in the future it is still tricky to decide what status update manager should keep track of reliably sending the update. I think it would make sense to bookkeep this operation in a failed state to the agent so its status update manager can in the future deliver the offer operation status to the framework.
For now we could just drop operations without an extractable resource provider id here with some logging and a `TODO`.
src/slave/slave.cpp
Lines 7412 (patched)
<https://reviews.apache.org/r/64477/#comment271936>
See comment on `Slave::addOfferOperation`.
src/slave/slave.cpp
Lines 7444 (patched)
<https://reviews.apache.org/r/64477/#comment271925>
Let's add a `CHECK` here that `info.has_id()`.
- Benjamin Bannier
On Dec. 10, 2017, 6:11 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64477/
> -----------------------------------------------------------
>
> (Updated Dec. 10, 2017, 6:11 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Greg Mann, and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Currently, we don't explicitly keep track of local resources providers.
> This causes the logic for a few methods to be quite complex because we
> need to reconstruct the resource provider information everytime.
>
>
> Diffs
> -----
>
> src/resource_provider/manager.cpp f98611c351f11b15281dd3ef673ef80db1c44f07
> src/resource_provider/message.hpp bbf6bb2a0a4aa901ac038cdbe4dd46acc0f2e453
> src/slave/http.cpp 738786fc4b85903f187cf0988a3fca488ea2767d
> src/slave/slave.hpp b3a1e70169ed1abbfc815821771715197a63f2df
> src/slave/slave.cpp 373e393ca1e7c0c30c3474cc9e630e25ad92f235
> src/tests/resource_provider_manager_tests.cpp a6eb4c9a303780029244e069bdf550a8cd9c7bb4
>
>
> Diff: https://reviews.apache.org/r/64477/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 64477: Refactored agent to keep track of local
resource providers.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64477/#review193426
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Bannier
On Dec. 11, 2017, 7:44 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64477/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2017, 7:44 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Greg Mann, and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Currently, we don't explicitly keep track of local resources providers.
> This causes the logic for a few methods to be quite complex because we
> need to reconstruct the resource provider information everytime.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 38d9a3c4204cfd6c93dde9216325db4d00cd5364
> src/resource_provider/manager.cpp f98611c351f11b15281dd3ef673ef80db1c44f07
> src/resource_provider/message.hpp bbf6bb2a0a4aa901ac038cdbe4dd46acc0f2e453
> src/slave/http.cpp 738786fc4b85903f187cf0988a3fca488ea2767d
> src/slave/slave.hpp b3a1e70169ed1abbfc815821771715197a63f2df
> src/slave/slave.cpp 373e393ca1e7c0c30c3474cc9e630e25ad92f235
> src/tests/resource_provider_manager_tests.cpp a6eb4c9a303780029244e069bdf550a8cd9c7bb4
>
>
> Diff: https://reviews.apache.org/r/64477/diff/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 64477: Refactored agent to keep track of local
resource providers.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64477/
-----------------------------------------------------------
(Updated Dec. 11, 2017, 6:44 p.m.)
Review request for mesos, Benjamin Bannier, Greg Mann, and Jan Schlicht.
Changes
-------
Addressed comments.
Repository: mesos
Description
-------
Currently, we don't explicitly keep track of local resources providers.
This causes the logic for a few methods to be quite complex because we
need to reconstruct the resource provider information everytime.
Diffs (updated)
-----
src/master/validation.cpp 38d9a3c4204cfd6c93dde9216325db4d00cd5364
src/resource_provider/manager.cpp f98611c351f11b15281dd3ef673ef80db1c44f07
src/resource_provider/message.hpp bbf6bb2a0a4aa901ac038cdbe4dd46acc0f2e453
src/slave/http.cpp 738786fc4b85903f187cf0988a3fca488ea2767d
src/slave/slave.hpp b3a1e70169ed1abbfc815821771715197a63f2df
src/slave/slave.cpp 373e393ca1e7c0c30c3474cc9e630e25ad92f235
src/tests/resource_provider_manager_tests.cpp a6eb4c9a303780029244e069bdf550a8cd9c7bb4
Diff: https://reviews.apache.org/r/64477/diff/3/
Changes: https://reviews.apache.org/r/64477/diff/2-3/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 64477: Refactored agent to keep track of local
resource providers.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64477/
-----------------------------------------------------------
(Updated Dec. 10, 2017, 5:11 p.m.)
Review request for mesos, Benjamin Bannier, Greg Mann, and Jan Schlicht.
Changes
-------
Rebased.
Repository: mesos
Description
-------
Currently, we don't explicitly keep track of local resources providers.
This causes the logic for a few methods to be quite complex because we
need to reconstruct the resource provider information everytime.
Diffs (updated)
-----
src/resource_provider/manager.cpp f98611c351f11b15281dd3ef673ef80db1c44f07
src/resource_provider/message.hpp bbf6bb2a0a4aa901ac038cdbe4dd46acc0f2e453
src/slave/http.cpp 738786fc4b85903f187cf0988a3fca488ea2767d
src/slave/slave.hpp b3a1e70169ed1abbfc815821771715197a63f2df
src/slave/slave.cpp 373e393ca1e7c0c30c3474cc9e630e25ad92f235
src/tests/resource_provider_manager_tests.cpp a6eb4c9a303780029244e069bdf550a8cd9c7bb4
Diff: https://reviews.apache.org/r/64477/diff/2/
Changes: https://reviews.apache.org/r/64477/diff/1-2/
Testing
-------
make check
Thanks,
Jie Yu