> On Dec. 4, 2018, 11:20 p.m., James DeFelice wrote: > > include/mesos/v1/mesos.proto > > Lines 2434 (patched) > > <https://reviews.apache.org/r/69162/diff/7/?file=2111243#file2111243line2434> > > > > I'd like this part to be a bit more fleshed out: > > > > When `uuid` is set then it MUST be possible to acknowledge the status > > update by using the specified `agent_id` and `resource_provider_id` (for > > local providers); and `resource_provider_id` (for external providers). > > Benjamin Bannier wrote: > I'd argue that this is already implied by the documentation of `uuid`, > ```` > // Statuses that are delivered reliably to the scheduler will > // include a `uuid`. The status is considered delivered once > // it is acknowledged by the scheduler. > optional UUID uuid = 5; > ```` > > Dropping for now. > > James DeFelice wrote: > I see what you mean by it being implied. However, I'd really like for it > to be more explicitly documented so that it's not accidentally overlooked > later. > > Greg Mann wrote: > I'm not sure that I understand the suggestion. Does the statement "When > uuid is set then it MUST be possible to acknowledge the status update by > using the specified agent_id and resource_provider_id" imply that when `uuid` > is set, `agent_id` and `resource_provider_id` must best set (or just RP ID in > the case of ext. providers)? That is not the case, as we will support both > operations on agent default resources (no RP ID) and operations by operators > (no agent ID) in the future. > > James, is the following an accurate representation of your intended > meaning? - > > "When `uuid` is set then it MUST be possible to acknowledge the status > update by using the specified `agent_id` and/or `resource_provider_id`, when > present." > > > > I'm also not sure why this comment would mention acknowledgement - isn't > it true that the desired constraint here is that it should be possible for > any update which contains a `uuid` to be _reconciled_ by the framework using > the supplied agent/RP ID, when present? > > James DeFelice wrote: > The goal is to document that Mesos will never provide a framework a UUID > that cannot be acknowledged due to informtion that's missing in the status > update proto.
I'm not sure that a comment here is necessary to accomplish that. I might update the comments in the `AcknowledgeOperationStatus` message in the scheduler API to say: ``` message AcknowledgeOperationStatus { // If either `agent_id` or `resource_provider_id` are set in the // operation status received by the scheduler, they should be set // here as well. optional AgentID agent_id = 1; optional ResourceProviderID resource_provider_id = 2; required bytes uuid = 3; required OperationID operation_id = 4; } ``` This would provide explicit instructions to framework devs to include agent and/or RP IDs in the acknowledgement any time they're set in the status. WDYT? - Greg ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69162/#review211030 ----------------------------------------------------------- On Dec. 5, 2018, noon, Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69162/ > ----------------------------------------------------------- > > (Updated Dec. 5, 2018, noon) > > > Review request for mesos, Chun-Hung Hsiao, Gastón Kleiman, and James DeFelice. > > > Bugs: MESOS-9293 > https://issues.apache.org/jira/browse/MESOS-9293 > > > Repository: mesos > > > Description > ------- > > This patch adds agent and resource provider IDs to > `UpdateOperationStatus` and `UpdateOperationStatusMessage`. With that > frameworks are able to reconcile enough information after failover to > construct operation acknowledgements. > > We will add code to populate these fields in a follow-up patch. > > > Diffs > ----- > > include/mesos/mesos.proto c822cc747cf153435b6c3ae1004168d5a289c97b > include/mesos/v1/mesos.proto 51c1bfdbb360b3554eba2229ed386d6271e0315b > src/common/type_utils.cpp ef13eae47b88efc15f1b2d00852b6387c2fffcbc > src/internal/devolve.cpp 491ed2aa131a92e958bfa71cccfc5f257cd4b3f9 > src/internal/evolve.cpp aa60efed5a28ed4d847a4a27aa8e994cda82175d > > > Diff: https://reviews.apache.org/r/69162/diff/8/ > > > Testing > ------- > > `make check` > > > Thanks, > > Benjamin Bannier > >