----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69569/#review211342 -----------------------------------------------------------
src/common/protobuf_utils.cpp Lines 470 (patched) <https://reviews.apache.org/r/69569/#comment296325> Since we set the agent ID and RP ID here in SLRP, how about doing the following changes: 1) Change https://github.com/apache/mesos/blob/b6fc9706c03c93239128f4a27051997c9647d8ff/src/slave/slave.cpp#L7859-L7869 to: ``` // Inject the agent ID into the status update if the resource // provider does not provide it (because the resource provider // may not know it). ... if (!update.has_slave_id()) { update.mutable_slave_id()->CopyFrom(...); } if (update.has_latest_status() && !update.latest_status().has_slave_id()) { update.mutable_latest_status()->mutable_slave_id()->CopyFrom(...); } ``` 2) Change https://github.com/apache/mesos/blob/b6fc9706c03c93239128f4a27051997c9647d8ff/src/resource_provider/manager.cpp#L886-L893 to: ``` CHECK_EQ(update.status().resource_provider_id(), resourceProvider->info().id()); ... if (update.has_latest_status()) { CHECK_EQ(update.latest_status().resource_provider_id(), resourceProvider->info().id()); body.update.mutable_latest_status()->CopyFrom(...); } ``` Then a followup work would be introduce another event to surface these errors and shutdown the RP instead of crash the RP manager. Could you create a ticket for this? Or alternatively, make the agent ID injection code into CHECKs since `LocalResourceProviderDaemon` provides the agent ID when creating LRPs. You might argue that it leaks informations to RP, but my counter argument would be that an LRP needs to learn about the agent lifecycle so it can know when to subscribe with no ID and when to resubscribe with a new ID, so it either needs to learn about the current agent ID, or we need provide feedback through the HTTP response of the `SUBSCRIBE` call. We could pick either approach for now. src/tests/storage_local_resource_provider_tests.cpp Lines 3459 (patched) <https://reviews.apache.org/r/69569/#comment296326> Nit: This can be a const reference. - Chun-Hung Hsiao On Dec. 14, 2018, 3:17 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69569/ > ----------------------------------------------------------- > > (Updated Dec. 14, 2018, 3:17 p.m.) > > > Review request for mesos, Chun-Hung Hsiao and Greg Mann. > > > Bugs: MESOS-9479 > https://issues.apache.org/jira/browse/MESOS-9479 > > > Repository: mesos > > > Description > ------- > > The helper function to set up the message was set up to receive > arguments, but we missed implementing the code making use of the values. > This went undiscovered as previously only used a mock resource provider. > > > Diffs > ----- > > src/common/protobuf_utils.cpp 583f0ab0dd874602bbd20731f1f32116a425ebc4 > src/tests/master_tests.cpp 3bff7a13bb8013ef91b72c14ace23ee94bc190e9 > src/tests/storage_local_resource_provider_tests.cpp > 7887bd5ec300be3cc093886525d71b3e09bbf085 > > > Diff: https://reviews.apache.org/r/69569/diff/1/ > > > Testing > ------- > > * `make check` > * ran added test in repeatition, both with and without additional stress on > the system > > > Thanks, > > Benjamin Bannier > >
