----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63622/#review190355 -----------------------------------------------------------
src/resource_provider/message.hpp Lines 48-52 (patched) <https://reviews.apache.org/r/63622/#comment267685> Given this is really just `OfferOperationStatusUpdate` and we'll need to construct that in the agent anyway, i'd prefer to use just that: ``` struct UpdateOfferOperationStatus { OfferOperationStatusUpdate update; }; ``` I was debating if we need to include `ResourceProviderID` here or not. One idea is to add an optional resource_provider_id in `OfferOperationStatusUpdate` in the future for ERP. We don't have to do it now. src/slave/slave.hpp Lines 670 (patched) <https://reviews.apache.org/r/63622/#comment267686> This is related to if we want to do bookkeeping for Offer operations in the agent. Given that in `UpdateState` call, RP will inform the RP manager about all known `OfferOperation`s, i think it makes sense to bookkeep `OfferOperation`s in the agent too (instead of just Offer::Operation). We probably need to mimic what we do in the master for bookkeeping. Think about a agent failover, this information will be lost and agent needs to reconstruct that information. Otherwise, when it receives a status udpate about an offer operation, it'll drop it according to the current logic below! src/slave/slave.cpp Lines 3731 (patched) <https://reviews.apache.org/r/63622/#comment267687> Ditto. THis will probably be `addOfferOperation(...)` src/slave/slave.cpp Lines 6813 (patched) <https://reviews.apache.org/r/63622/#comment267693> You cannot remove this if the latest status is not terminal. src/slave/slave.cpp Lines 6823-6825 (patched) <https://reviews.apache.org/r/63622/#comment267692> We still want to forward the status udpate for those operations, right? Just we don't need to adjust the `totalResources` of the agent. I think a better flow here is: ``` vector<ResourceConversion> conversions; // Calculate conversions based on the status update. // This only applies to new operations and latest // status is terminal. For other cases, conversions // will be empty. totalResources.apply(conversions); // Forward status update. ``` src/slave/slave.cpp Lines 6846 (patched) <https://reviews.apache.org/r/63622/#comment267694> we should look at `latest_status`. src/slave/slave.cpp Lines 6847 (patched) <https://reviews.apache.org/r/63622/#comment267688> Let's do the subtraction first (and a contains check as Benjamin suggested). src/slave/slave.cpp Lines 6861-6865 (patched) <https://reviews.apache.org/r/63622/#comment267695> Let's log a warning here (dropping status update). Status update manager in the RP will retry the status update eventually. - Jie Yu On Nov. 7, 2017, 2:20 p.m., Jan Schlicht wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63622/ > ----------------------------------------------------------- > > (Updated Nov. 7, 2017, 2:20 p.m.) > > > Review request for mesos, Benjamin Bannier and Jie Yu. > > > Repository: mesos > > > Description > ------- > > When a resource provider has finished an offer operation, it will send > a status update to the resource provider manager and subsequently to an > agent. The agent then updates its internal bookkeeping and forwards the > status update to the master. > > > Diffs > ----- > > src/resource_provider/manager.cpp a878507d71a09a415d8a4573cf2b33c26c985451 > src/resource_provider/message.hpp 3c7c3f2baeb726e04edd6ffbb9784699d7afe521 > src/slave/slave.hpp df1b0205124555dcb6a0efa5c237f5e77fa2bdf7 > src/slave/slave.cpp c10823985154bac19f8952b94311a03b2b9b4ea1 > src/tests/resource_provider_manager_tests.cpp > 4008b1c751d6227b99adef756e95174d7d8a62f2 > > > Diff: https://reviews.apache.org/r/63622/diff/1/ > > > Testing > ------- > > make check > > > Thanks, > > Jan Schlicht > >
