----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63622/#review190671 -----------------------------------------------------------
Fix it, then Ship it! src/resource_provider/manager.cpp Line 418 (original), 418-430 (patched) <https://reviews.apache.org/r/63622/#comment268236> nits: this looks pretty jagged. I'd suggest the following: ``` ResourceProviderMessage::UpdateOfferOperationStatus body; body.id = resourceProvider->info().id(); body.update.mutable_framework_id()->CopyFrom(update.framework_id()); body.update.mutable_status()->COpyFrom(update.status()); body.update.set_operation_uuid(update.operation_uuid()); if (update.has_latest_status()) { body.update.mutable_latest_status()->CopyFrom(update.latest_status()); } ResourceProviderMessage message; message.type = ResourceProviderMessage::Type::UPDATE_OFFER_OPERATION_STATUS; message.updateOfferOperationStatus = std::move(body); ``` src/resource_provider/message.hpp Lines 51 (patched) <https://reviews.apache.org/r/63622/#comment268237> do you need this currently? If not, let's remove it. In the future, we might want to add `resource_provider_id` in `OfferOperationStatusUpdate` (for ERP). src/resource_provider/message.hpp Lines 89 (patched) <https://reviews.apache.org/r/63622/#comment268238> i would juse JSON here. Debug string might across multiple lines? src/slave/slave.hpp Lines 541 (patched) <https://reviews.apache.org/r/63622/#comment268235> indentation src/slave/slave.cpp Lines 6792 (patched) <https://reviews.apache.org/r/63622/#comment268239> CHECK_SOME src/slave/slave.cpp Lines 6809 (patched) <https://reviews.apache.org/r/63622/#comment268240> Can you make the logging more complete. e.g., which framework, and what's the id and uuid of the operation? src/slave/slave.cpp Lines 6813 (patched) <https://reviews.apache.org/r/63622/#comment268241> Ditto. src/slave/slave.cpp Lines 6818 (patched) <https://reviews.apache.org/r/63622/#comment268242> `s/update_/_update/` src/slave/slave.cpp Lines 6826-6830 (patched) <https://reviews.apache.org/r/63622/#comment268243> I don't think you can remove the operation until an ack is received for the terminal status update. Otherwise, if agent gets disconnected -> message lost -> RP retried -> unknown operation. For now, I think it's OK because RP will not retry. If message get lost, given that the total has been converted already, not sending this operation to the master sounds ok. Can you leave a TODO here saying that we need to adjust the logic here once we add retry and ack support? please remove the `has_id` check here. I don't think this matters here. Even if framework does not ack, we still need ack from the master to remove the operation. src/slave/slave.cpp Lines 6933-6949 (patched) <https://reviews.apache.org/r/63622/#comment268244> Similar to that in Master. I'd factor this into a helper called `Slave::apply(const vector<ResourceConversion>& conversions)` - Jie Yu On Nov. 9, 2017, 1:48 p.m., Jan Schlicht wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63622/ > ----------------------------------------------------------- > > (Updated Nov. 9, 2017, 1:48 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/common/protobuf_utils.hpp 0ca4c6d689bf7d2c477174b039432ed7b6d0b650 > src/common/protobuf_utils.cpp 5739a63f8d87923c034375b88c4f0b3b19f4b521 > src/resource_provider/manager.cpp bcc833b6c96344762a693dd6d4e0a9129aa735f8 > src/resource_provider/message.hpp a1a84c1fd374b740c56ed2175ee5a2dbc8f96dbc > src/slave/slave.hpp 0124df44256685689b593dfbf962e4482e4495c6 > src/slave/slave.cpp 7cb6661b55fb5437a1ffc447f974076aadd1eced > src/tests/resource_provider_manager_tests.cpp > 4008b1c751d6227b99adef756e95174d7d8a62f2 > > > Diff: https://reviews.apache.org/r/63622/diff/4/ > > > Testing > ------- > > make check > > > Thanks, > > Jan Schlicht > >
