You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Bannier <be...@mesosphere.io> on 2018/10/25 10:54:38 UTC
Review Request 69162: Added agent and resource provider IDs to
operation status messages.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69162/
-----------------------------------------------------------
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 add agent and resource provide 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/scheduler/scheduler.proto f6a780a7b75878b9e74402a28a25bb868f7ac36f
include/mesos/v1/scheduler/scheduler.proto fcfec5e417463103e98dd6917722b4fde41cac7c
src/internal/evolve.cpp aa60efed5a28ed4d847a4a27aa8e994cda82175d
src/messages/messages.cpp dd8f60ecdbc06d10be1152bee1ddb65feaaf8fbb
src/messages/messages.proto 41e6a8a2eab0ae7c2878c1d3286c5dea0eb68ed7
Diff: https://reviews.apache.org/r/69162/diff/1/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 69162: Added agent and resource provider IDs to
operation status messages.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Nov. 20, 2018, 2:57 a.m., Chun-Hung Hsiao wrote:
> > include/mesos/scheduler/scheduler.proto
> > Line 140 (original), 140 (patched)
> > <https://reviews.apache.org/r/69162/diff/3/?file=2106989#file2106989line140>
> >
> > It seems more consistent with `TaskStatus` if we put `slave_id` and `resource_provider_id` in `OperationStatus`. What's the pros and cons of doing that instead?
We discussed this offline, and I moved the information into `TaskStatus`.
The gist of the discussion was that moving it into `OperationStatus` instead of `UpdateOperationStatus` introduces less coupling and imposes less future restrictions (just like for `TaskStatus`).
> On Nov. 20, 2018, 2:57 a.m., Chun-Hung Hsiao wrote:
> > src/messages/messages.cpp
> > Lines 146-149 (original), 156-162 (patched)
> > <https://reviews.apache.org/r/69162/diff/3/?file=2106992#file2106992line156>
> >
> > How about the following:
> > ```
> > if (update.has_resource_provider_id()) {
> > stream << " from resource provider " << update.resource_provider_id();
> > }
> >
> > if (update.has_slave_id()) {
> > stream << " on agent " << update.slave_id();
> > }
> > ```
Not an issue anymore.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69162/#review210694
-----------------------------------------------------------
On Nov. 12, 2018, 9:49 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69162/
> -----------------------------------------------------------
>
> (Updated Nov. 12, 2018, 9:49 p.m.)
>
>
> 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 add agent and resource provide 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/scheduler/scheduler.proto f6a780a7b75878b9e74402a28a25bb868f7ac36f
> include/mesos/v1/scheduler/scheduler.proto fcfec5e417463103e98dd6917722b4fde41cac7c
> src/internal/evolve.cpp aa60efed5a28ed4d847a4a27aa8e994cda82175d
> src/messages/messages.cpp dd8f60ecdbc06d10be1152bee1ddb65feaaf8fbb
> src/messages/messages.proto 41e6a8a2eab0ae7c2878c1d3286c5dea0eb68ed7
>
>
> Diff: https://reviews.apache.org/r/69162/diff/4/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69162: Added agent and resource provider IDs to
operation status messages.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69162/#review210694
-----------------------------------------------------------
include/mesos/scheduler/scheduler.proto
Line 140 (original), 140 (patched)
<https://reviews.apache.org/r/69162/#comment295424>
It seems more consistent with `TaskStatus` if we put `slave_id` and `resource_provider_id` in `OperationStatus`. What's the pros and cons of doing that instead?
include/mesos/scheduler/scheduler.proto
Lines 146 (patched)
<https://reviews.apache.org/r/69162/#comment295416>
s/ a / an /
include/mesos/scheduler/scheduler.proto
Lines 149 (patched)
<https://reviews.apache.org/r/69162/#comment295422>
Should we be explicit about what cases they are? If it's not possible, maybe say the following instead?
```
// In certain cases, e.g., invalid operations, neither `slave_id` nor
// `resource_provider_id` will be set, and the scheduler does not need to
// acknowledge this status update.
```
include/mesos/v1/scheduler/scheduler.proto
Lines 144 (patched)
<https://reviews.apache.org/r/69162/#comment295420>
s/ a / an /
include/mesos/v1/scheduler/scheduler.proto
Lines 147 (patched)
<https://reviews.apache.org/r/69162/#comment295423>
Ditto.
src/messages/messages.cpp
Lines 82-84 (patched)
<https://reviews.apache.org/r/69162/#comment295417>
Nit: How about following what clang-format gives us here?
```
if (left.has_resource_provider_id() &&
left.resource_provider_id() != right.resource_provider_id()) {
```
The conditions and the body can be easily distinguished by different indentations above.
src/messages/messages.cpp
Lines 146-149 (original), 156-162 (patched)
<https://reviews.apache.org/r/69162/#comment295418>
How about the following:
```
if (update.has_resource_provider_id()) {
stream << " from resource provider " << update.resource_provider_id();
}
if (update.has_slave_id()) {
stream << " on agent " << update.slave_id();
}
```
- Chun-Hung Hsiao
On Nov. 12, 2018, 8:49 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69162/
> -----------------------------------------------------------
>
> (Updated Nov. 12, 2018, 8:49 p.m.)
>
>
> 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 add agent and resource provide 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/scheduler/scheduler.proto f6a780a7b75878b9e74402a28a25bb868f7ac36f
> include/mesos/v1/scheduler/scheduler.proto fcfec5e417463103e98dd6917722b4fde41cac7c
> src/internal/evolve.cpp aa60efed5a28ed4d847a4a27aa8e994cda82175d
> src/messages/messages.cpp dd8f60ecdbc06d10be1152bee1ddb65feaaf8fbb
> src/messages/messages.proto 41e6a8a2eab0ae7c2878c1d3286c5dea0eb68ed7
>
>
> Diff: https://reviews.apache.org/r/69162/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69162: Added agent and resource provider IDs to
operation status messages.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69162/#review210935
-----------------------------------------------------------
Fix it, then Ship it!
In commit message: s/This patch add agent and resource provide IDs to/This patch adds agent and resource provider IDs to/
include/mesos/mesos.proto
Lines 2401-2403 (patched)
<https://reviews.apache.org/r/69162/#comment295784>
I would be explicit here that if both fields are unset, then `uuid` should be unset also.
- Greg Mann
On Nov. 27, 2018, 6:03 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69162/
> -----------------------------------------------------------
>
> (Updated Nov. 27, 2018, 6:03 p.m.)
>
>
> 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 add agent and resource provide 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 56107f47a46ac3679a57af0580c55ad0f98543f5
> include/mesos/v1/mesos.proto c6e7515a44eca0b057725c7b8196250072b56be5
> src/common/type_utils.cpp ef13eae47b88efc15f1b2d00852b6387c2fffcbc
> src/internal/devolve.cpp 491ed2aa131a92e958bfa71cccfc5f257cd4b3f9
> src/internal/evolve.cpp aa60efed5a28ed4d847a4a27aa8e994cda82175d
>
>
> Diff: https://reviews.apache.org/r/69162/diff/6/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69162: Added agent and resource provider IDs to
operation status messages.
Posted by Greg Mann <gr...@mesosphere.io>.
> 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.
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?
- 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
>
>
Re: Review Request 69162: Added agent and resource provider IDs to
operation status messages.
Posted by James DeFelice <ja...@gmail.com>.
> 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?
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.
- James
-----------------------------------------------------------
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
>
>
Re: Review Request 69162: Added agent and resource provider IDs to
operation status messages.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Dec. 5, 2018, 12:20 a.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).
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.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69162/#review211030
-----------------------------------------------------------
On Dec. 5, 2018, 1 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69162/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2018, 1 p.m.)
>
>
> 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
>
>
Re: Review Request 69162: Added agent and resource provider IDs to
operation status messages.
Posted by James DeFelice <ja...@gmail.com>.
> 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.
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.
- James
-----------------------------------------------------------
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
>
>
Re: Review Request 69162: Added agent and resource provider IDs to
operation status messages.
Posted by James DeFelice <ja...@gmail.com>.
> 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.
>
> Greg Mann wrote:
> 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?
I like adding a comment there too. I guess I was looking to explicitly document (via API) a guarantee that Mesos will always provide the ID fields necessary in order for a framework to properly ACK when uuid is set. As per a PM w/ Benjamin, unit tests could help codify guarantee as well (and prevent accidental regression). He's filed a follow-up JIRA for such tests.
- James
-----------------------------------------------------------
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
>
>
Re: Review Request 69162: Added agent and resource provider IDs to
operation status messages.
Posted by Greg Mann <gr...@mesosphere.io>.
> 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
>
>
Re: Review Request 69162: Added agent and resource provider IDs to
operation status messages.
Posted by James DeFelice <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69162/#review211030
-----------------------------------------------------------
include/mesos/v1/mesos.proto
Lines 2428 (patched)
<https://reviews.apache.org/r/69162/#comment295881>
s/slave_id/agent_id/
include/mesos/v1/mesos.proto
Lines 2434 (patched)
<https://reviews.apache.org/r/69162/#comment295883>
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).
- James DeFelice
On Nov. 30, 2018, 11:06 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69162/
> -----------------------------------------------------------
>
> (Updated Nov. 30, 2018, 11:06 a.m.)
>
>
> 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/7/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69162: Added agent and resource provider IDs to
operation status messages.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69162/
-----------------------------------------------------------
(Updated Dec. 5, 2018, 1 p.m.)
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 (updated)
-----
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/
Changes: https://reviews.apache.org/r/69162/diff/7-8/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 69162: Added agent and resource provider IDs to
operation status messages.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69162/#review210988
-----------------------------------------------------------
In the first line of the commit message: s/provide/provider/ :)
- Greg Mann
On Nov. 30, 2018, 11:06 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69162/
> -----------------------------------------------------------
>
> (Updated Nov. 30, 2018, 11:06 a.m.)
>
>
> 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 provide 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/7/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69162: Added agent and resource provider IDs to
operation status messages.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69162/
-----------------------------------------------------------
(Updated Nov. 30, 2018, 12:06 p.m.)
Review request for mesos, Chun-Hung Hsiao, Gastón Kleiman, and James DeFelice.
Changes
-------
Addressed Greg's comments.
Bugs: MESOS-9293
https://issues.apache.org/jira/browse/MESOS-9293
Repository: mesos
Description (updated)
-------
This patch adds agent and resource provide 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 (updated)
-----
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/7/
Changes: https://reviews.apache.org/r/69162/diff/6-7/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 69162: Added agent and resource provider IDs to
operation status messages.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69162/
-----------------------------------------------------------
(Updated Nov. 27, 2018, 7:03 p.m.)
Review request for mesos, Chun-Hung Hsiao, Gastón Kleiman, and James DeFelice.
Changes
-------
Moved information into `OperationStatus` as suggested by Chun.
Bugs: MESOS-9293
https://issues.apache.org/jira/browse/MESOS-9293
Repository: mesos
Description
-------
This patch add agent and resource provide 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 (updated)
-----
include/mesos/mesos.proto 06a901d26693757edc653cd833d55aa42e4ff2c6
include/mesos/v1/mesos.proto 75cdb2889b2b645e23d9f5ab263ee63bf62b4221
src/common/type_utils.cpp ef13eae47b88efc15f1b2d00852b6387c2fffcbc
Diff: https://reviews.apache.org/r/69162/diff/5/
Changes: https://reviews.apache.org/r/69162/diff/4-5/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 69162: Added agent and resource provider IDs to
operation status messages.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69162/
-----------------------------------------------------------
(Updated Nov. 12, 2018, 9:49 p.m.)
Review request for mesos, Chun-Hung Hsiao, Gastón Kleiman, and James DeFelice.
Changes
-------
Rebased.
Bugs: MESOS-9293
https://issues.apache.org/jira/browse/MESOS-9293
Repository: mesos
Description
-------
This patch add agent and resource provide 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 (updated)
-----
include/mesos/scheduler/scheduler.proto f6a780a7b75878b9e74402a28a25bb868f7ac36f
include/mesos/v1/scheduler/scheduler.proto fcfec5e417463103e98dd6917722b4fde41cac7c
src/internal/evolve.cpp aa60efed5a28ed4d847a4a27aa8e994cda82175d
src/messages/messages.cpp dd8f60ecdbc06d10be1152bee1ddb65feaaf8fbb
src/messages/messages.proto 41e6a8a2eab0ae7c2878c1d3286c5dea0eb68ed7
Diff: https://reviews.apache.org/r/69162/diff/3/
Changes: https://reviews.apache.org/r/69162/diff/2-3/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 69162: Added agent and resource provider IDs to
operation status messages.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Oct. 25, 2018, 2:13 p.m., James DeFelice wrote:
> > include/mesos/v1/scheduler/scheduler.proto
> > Lines 147 (patched)
> > <https://reviews.apache.org/r/69162/diff/1/?file=2102442#file2102442line147>
> >
> > For these "certain cases" does Mesos still expect an ACK? If so, that's a problem.
No, it currently does not as these updates are just sent from the master not a status update manager.
Dropping.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69162/#review210031
-----------------------------------------------------------
On Oct. 25, 2018, 12:54 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69162/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2018, 12:54 p.m.)
>
>
> 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 add agent and resource provide 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/scheduler/scheduler.proto f6a780a7b75878b9e74402a28a25bb868f7ac36f
> include/mesos/v1/scheduler/scheduler.proto fcfec5e417463103e98dd6917722b4fde41cac7c
> src/internal/evolve.cpp aa60efed5a28ed4d847a4a27aa8e994cda82175d
> src/messages/messages.cpp dd8f60ecdbc06d10be1152bee1ddb65feaaf8fbb
> src/messages/messages.proto 41e6a8a2eab0ae7c2878c1d3286c5dea0eb68ed7
>
>
> Diff: https://reviews.apache.org/r/69162/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69162: Added agent and resource provider IDs to
operation status messages.
Posted by James DeFelice <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69162/#review210031
-----------------------------------------------------------
include/mesos/v1/scheduler/scheduler.proto
Lines 147 (patched)
<https://reviews.apache.org/r/69162/#comment294673>
For these "certain cases" does Mesos still expect an ACK? If so, that's a problem.
- James DeFelice
On Oct. 25, 2018, 10:54 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69162/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2018, 10:54 a.m.)
>
>
> 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 add agent and resource provide 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/scheduler/scheduler.proto f6a780a7b75878b9e74402a28a25bb868f7ac36f
> include/mesos/v1/scheduler/scheduler.proto fcfec5e417463103e98dd6917722b4fde41cac7c
> src/internal/evolve.cpp aa60efed5a28ed4d847a4a27aa8e994cda82175d
> src/messages/messages.cpp dd8f60ecdbc06d10be1152bee1ddb65feaaf8fbb
> src/messages/messages.proto 41e6a8a2eab0ae7c2878c1d3286c5dea0eb68ed7
>
>
> Diff: https://reviews.apache.org/r/69162/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>