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