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:39 UTC
Review Request 69163: Set agent and/or resource provider ID in
operation status updates.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69163/
-----------------------------------------------------------
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 sets agent and/or resource provider ID operation status
update messages. This is not always possible, e.g., some operations
might fail validation so that no corresponding IDs can be extracted.
Since operations failing validation are currently directly rejected by
the master without going through a status update manager, they are not
retried either. If a master status update manager for operations is
introduced at a later point it should be possible to forward
acknowledgements for updates to the master's update manager (no agent
ID, not resource provider ID).
Diffs
-----
src/common/protobuf_utils.hpp 1662125ed3e47b179ee32d08c1d3af75553066ba
src/common/protobuf_utils.cpp 77139d8a3931dc1e9e00fcea83d77d96244a34f3
src/master/master.cpp 0c95c438975efd949cbf86f7d8bfea940c20a43a
src/resource_provider/storage/provider.cpp db783b53558811081fb2671e005e8bbbd9edbede
src/slave/slave.cpp 7bb2b291a7b67563adf88bcedca1392890f17ffe
src/tests/slave_tests.cpp 637bedce20f63dfc1702ef549bcea8f790477be3
Diff: https://reviews.apache.org/r/69163/diff/1/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 69163: Set agent and/or resource provider ID in
operation status updates.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69163/#review210234
-----------------------------------------------------------
Patch looks great!
Reviews applied: [69159, 69160, 68147, 69157, 69158, 69161, 69162, 69163]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
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/69163/
> -----------------------------------------------------------
>
> (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 sets agent and/or resource provider ID operation status
> update messages. This is not always possible, e.g., some operations
> might fail validation so that no corresponding IDs can be extracted.
>
> Since operations failing validation are currently directly rejected by
> the master without going through a status update manager, they are not
> retried either. If a master status update manager for operations is
> introduced at a later point it should be possible to forward
> acknowledgements for updates to the master's update manager (no agent
> ID, not resource provider ID).
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 1662125ed3e47b179ee32d08c1d3af75553066ba
> src/common/protobuf_utils.cpp 2ae37ac610894652a0214022a0cf04c4365396dd
> src/master/master.cpp f88c7c1f03f0de7236aad9e3bf4bfac82e91bc65
> src/resource_provider/storage/provider.cpp db783b53558811081fb2671e005e8bbbd9edbede
> src/slave/slave.cpp 74f6fb9036a9ac4f587f53ec2df04eeb4c167bfb
> src/tests/slave_tests.cpp 637bedce20f63dfc1702ef549bcea8f790477be3
>
>
> Diff: https://reviews.apache.org/r/69163/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69163: Set agent and/or resource provider ID in
operation status updates.
Posted by James DeFelice <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69163/#review210237
-----------------------------------------------------------
> This patch sets agent and/or resource provider ID operation status
update messages. This is not always possible, e.g., some operations
might fail validation so that no corresponding IDs can be extracted
Will the framework be expected to ACK in these cases? If so, missing the IDs is problematic.
- 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/69163/
> -----------------------------------------------------------
>
> (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 sets agent and/or resource provider ID operation status
> update messages. This is not always possible, e.g., some operations
> might fail validation so that no corresponding IDs can be extracted.
>
> Since operations failing validation are currently directly rejected by
> the master without going through a status update manager, they are not
> retried either. If a master status update manager for operations is
> introduced at a later point it should be possible to forward
> acknowledgements for updates to the master's update manager (no agent
> ID, not resource provider ID).
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 1662125ed3e47b179ee32d08c1d3af75553066ba
> src/common/protobuf_utils.cpp 2ae37ac610894652a0214022a0cf04c4365396dd
> src/master/master.cpp f88c7c1f03f0de7236aad9e3bf4bfac82e91bc65
> src/resource_provider/storage/provider.cpp db783b53558811081fb2671e005e8bbbd9edbede
> src/slave/slave.cpp 74f6fb9036a9ac4f587f53ec2df04eeb4c167bfb
> src/tests/slave_tests.cpp 637bedce20f63dfc1702ef549bcea8f790477be3
>
>
> Diff: https://reviews.apache.org/r/69163/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69163: Set agent and/or resource provider ID in
operation status updates.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Nov. 20, 2018, 4:24 a.m., Chun-Hung Hsiao wrote:
> > src/master/master.cpp
> > Lines 8192-8201 (patched)
> > <https://reviews.apache.org/r/69163/diff/3/?file=2106996#file2106996line8192>
> >
> > We can get rid of this snippet and simply use `providerId`. Or, validate that `resourceProviderId` is always the same as `providerId`.
> >
> > We should probably validate that both resources and operations have their resource provider id set properly in the resource provider manager, and issue an `ERROR` event back to the resource provider, instead of crashing the agent: https://github.com/apache/mesos/blob/master/src/resource_provider/manager.cpp#L907
> > I created https://issues.apache.org/jira/browse/MESOS-9407 to capture this. No need to address it in this patch.
Dropping since this is not an issue anymore in the updated code.
> On Nov. 20, 2018, 4:24 a.m., Chun-Hung Hsiao wrote:
> > src/master/master.cpp
> > Lines 8199 (patched)
> > <https://reviews.apache.org/r/69163/diff/3/?file=2106996#file2106996line8199>
> >
> > How about inlining this ternary operation into the use below so we can get rid of `resourceProviderId_`?
Ditto.
> On Nov. 20, 2018, 4:24 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/manager.cpp
> > Lines 887-888 (patched)
> > <https://reviews.apache.org/r/69163/diff/3/?file=2106997#file2106997line887>
> >
> > It seems not cohesive that we include both the agent ID and the resource provider ID when generating the `UpdateOperationStatusMessage` in SLRP, but drop them because `Call::UpdateOperationStatus` does not have corresponding fields, then:
> > 1. Recover the resource provider ID in the RP manager, and
> > 2. Recover the agent ID in `Slave::handleResourceProviderMessage`.
> >
> > I was wondering if it is a good idea to add `slave_id` and `resource_provider_id` in `OperationStatus` instead, then generate `OperationStatusUpdateMessage` based on `OperationStatus` for backward compatibility.
I did the suggested change in the preceeding patch, dropping.
> On Nov. 20, 2018, 4:24 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/storage/provider.cpp
> > Line 3079 (original), 3080-3081 (patched)
> > <https://reviews.apache.org/r/69163/diff/3/?file=2106998#file2106998line3080>
> >
> > We don't need to set up the slave ID and resource provider ID since dropped operations will not be bookkept and retried.
This piece of code is now gone, but I think it makes sense to provide semantics as consistent as possible to schedulers, so I'd prefer to set as many fields as possible, even if redundant.
Dropping.
> On Nov. 20, 2018, 4:24 a.m., Chun-Hung Hsiao wrote:
> > src/slave/slave.cpp
> > Lines 4386 (patched)
> > <https://reviews.apache.org/r/69163/diff/3/?file=2106999#file2106999line4386>
> >
> > How about inlining the ternary operation into its use below so we don't need this `resourceProviderId_`?
Reworked this code, droppping.
> On Nov. 20, 2018, 4:24 a.m., Chun-Hung Hsiao wrote:
> > src/tests/master_tests.cpp
> > Lines 9038 (patched)
> > <https://reviews.apache.org/r/69163/diff/3/?file=2107000#file2107000line9038>
> >
> > Since the resource provider manager is the one adding the resource provider ID, does it make sense to move this test to `resource_provider_manager_tests.cpp`?
> >
> > If we make the decision to add `slave_id` and `resource_provider_id` in `OperationStatus` and make the SLRP generates them, we should move this test to `storage_local_resource_provider_tests.cpp`.
I reworked the code so now RPs inject RP IDs, and agents inject agent IDs. The test here really only tests the agent part, as we use a `MockResourceProvider`. We could probably move it to `slave_tests.cpp`, but at the same time we also test an master API.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69163/#review210695
-----------------------------------------------------------
On Nov. 27, 2018, 7 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69163/
> -----------------------------------------------------------
>
> (Updated Nov. 27, 2018, 7 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 sets agent and/or resource provider ID operation status
> update messages. This is not always possible, e.g., some operations
> might fail validation so that no corresponding IDs can be extracted.
>
> Since operations failing validation are currently directly rejected by
> the master without going through a status update manager, they are not
> retried either. If a master status update manager for operations is
> introduced at a later point it should be possible to forward
> acknowledgements for updates to the master's update manager (no agent
> ID, not resource provider ID).
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 1662125ed3e47b179ee32d08c1d3af75553066ba
> src/common/protobuf_utils.cpp a45607eed4c4bae5010bcc3f3ffeabd6d911062a
> src/master/master.cpp b4b02d8b4d7d6d1aabda1f97b9bf824419f76a9e
> src/resource_provider/manager.cpp 6c81c430e9e1205d71982a7fa2bcd9aa15fc01b2
> src/resource_provider/storage/provider.cpp c137fa4f13edc58d93c03a9dd32fdf9d38b38316
> src/slave/slave.cpp 858b78620e1ef33f3587d0bd95a684917aaf5bbb
> src/tests/master_tests.cpp 651bb9ba9298fc3d7179ed86487aa55be519ce12
> src/tests/mesos.hpp 576f4bde88c069ee2fa0dd33912a034437338e7e
>
>
> Diff: https://reviews.apache.org/r/69163/diff/4/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69163: Set agent and/or resource provider ID in
operation status updates.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69163/#review210695
-----------------------------------------------------------
src/master/master.cpp
Lines 8192-8201 (patched)
<https://reviews.apache.org/r/69163/#comment295419>
We can get rid of this snippet and simply use `providerId`. Or, validate that `resourceProviderId` is always the same as `providerId`.
We should probably validate that both resources and operations have their resource provider id set properly in the resource provider manager, and issue an `ERROR` event back to the resource provider, instead of crashing the agent: https://github.com/apache/mesos/blob/master/src/resource_provider/manager.cpp#L907
I created https://issues.apache.org/jira/browse/MESOS-9407 to capture this. No need to address it in this patch.
src/master/master.cpp
Lines 8199 (patched)
<https://reviews.apache.org/r/69163/#comment295427>
How about inlining this ternary operation into the use below so we can get rid of `resourceProviderId_`?
src/resource_provider/manager.cpp
Lines 887-888 (patched)
<https://reviews.apache.org/r/69163/#comment295431>
It seems not cohesive that we include both the agent ID and the resource provider ID when generating the `UpdateOperationStatusMessage` in SLRP, but drop them because `Call::UpdateOperationStatus` does not have corresponding fields, then:
1. Recover the resource provider ID in the RP manager, and
2. Recover the agent ID in `Slave::handleResourceProviderMessage`.
I was wondering if it is a good idea to add `slave_id` and `resource_provider_id` in `OperationStatus` instead, then generate `OperationStatusUpdateMessage` based on `OperationStatus` for backward compatibility.
src/resource_provider/storage/provider.cpp
Line 3079 (original), 3080-3081 (patched)
<https://reviews.apache.org/r/69163/#comment295426>
We don't need to set up the slave ID and resource provider ID since dropped operations will not be bookkept and retried.
src/slave/slave.cpp
Lines 4386 (patched)
<https://reviews.apache.org/r/69163/#comment295428>
How about inlining the ternary operation into its use below so we don't need this `resourceProviderId_`?
src/tests/master_tests.cpp
Lines 9036 (patched)
<https://reviews.apache.org/r/69163/#comment295432>
We might want to backport this patch back to 1.7.x. Can you split the tests into another patch so we can minimize the backport?
src/tests/master_tests.cpp
Lines 9038 (patched)
<https://reviews.apache.org/r/69163/#comment295433>
Since the resource provider manager is the one adding the resource provider ID, does it make sense to move this test to `resource_provider_manager_tests.cpp`?
If we make the decision to add `slave_id` and `resource_provider_id` in `OperationStatus` and make the SLRP generates them, we should move this test to `storage_local_resource_provider_tests.cpp`.
src/tests/master_tests.cpp
Lines 9066-9070 (patched)
<https://reviews.apache.org/r/69163/#comment295429>
Nit: they fit into a line. ;)
- 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/69163/
> -----------------------------------------------------------
>
> (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 sets agent and/or resource provider ID operation status
> update messages. This is not always possible, e.g., some operations
> might fail validation so that no corresponding IDs can be extracted.
>
> Since operations failing validation are currently directly rejected by
> the master without going through a status update manager, they are not
> retried either. If a master status update manager for operations is
> introduced at a later point it should be possible to forward
> acknowledgements for updates to the master's update manager (no agent
> ID, not resource provider ID).
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 1662125ed3e47b179ee32d08c1d3af75553066ba
> src/common/protobuf_utils.cpp a45607eed4c4bae5010bcc3f3ffeabd6d911062a
> src/master/master.cpp 1e326ec42a7f79a0835529a4655e7ec272f1cf40
> src/resource_provider/manager.cpp 6c81c430e9e1205d71982a7fa2bcd9aa15fc01b2
> src/resource_provider/storage/provider.cpp c137fa4f13edc58d93c03a9dd32fdf9d38b38316
> src/slave/slave.cpp 74f6fb9036a9ac4f587f53ec2df04eeb4c167bfb
> src/tests/master_tests.cpp ac6bf379c5906cf9612284911c121c9457f648a0
>
>
> Diff: https://reviews.apache.org/r/69163/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69163: Set agent and/or resource provider ID in
operation status updates.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Nov. 30, 2018, 3:14 a.m., Greg Mann wrote:
> > src/master/master.cpp
> > Lines 2254-2255 (patched)
> > <https://reviews.apache.org/r/69163/diff/4/?file=2110688#file2110688line2254>
> >
> > It would make sense to me that we set these IDs if/when they are available. Currently I don't think it matters since the updates sent here do not need to be acknowledged; perhaps mention that in the comment?
I added a `TODO` to that effect. The issue currently is that we use `drop` to reject both "invalid" (e.g., invalid offer or resources) and "valid" operations (rejected because e.g., not authorized). For the latter we could set IDs in the terminal status.
> On Nov. 30, 2018, 3:14 a.m., Greg Mann wrote:
> > src/slave/slave.cpp
> > Line 4354 (original), 4353-4359 (patched)
> > <https://reviews.apache.org/r/69163/diff/4/?file=2110691#file2110691line4354>
> >
> > Why no RP ID? Here and below.
Good catch, fixed two more instances (third wasn't on RP resources).
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69163/#review210962
-----------------------------------------------------------
On Nov. 30, 2018, 12:06 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69163/
> -----------------------------------------------------------
>
> (Updated Nov. 30, 2018, 12:06 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 sets agent and/or resource provider ID operation status
> update messages. This is not always possible, e.g., some operations
> might fail validation so that no corresponding IDs can be extracted.
>
> Since operations failing validation are currently directly rejected by
> the master without going through a status update manager, they are not
> retried either. If a master status update manager for operations is
> introduced at a later point it should be possible to forward
> acknowledgements for updates to the master's update manager (not agent
> ID, not resource provider ID).
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 1662125ed3e47b179ee32d08c1d3af75553066ba
> src/common/protobuf_utils.cpp a45607eed4c4bae5010bcc3f3ffeabd6d911062a
> src/master/master.cpp 3b3824a67f46866cd64e32d7f9f92484b5891aa2
> src/resource_provider/manager.cpp 6c81c430e9e1205d71982a7fa2bcd9aa15fc01b2
> src/resource_provider/storage/provider.cpp a22c82c442304979fbdec0fcb74543077751a135
> src/slave/slave.cpp 858b78620e1ef33f3587d0bd95a684917aaf5bbb
> src/tests/master_tests.cpp ef2c00101fc3d30c564a9ca34884dece2cdd2651
> src/tests/mesos.hpp c08e7e6c1dbc3dd9eb980868e43368c0a423c3ee
>
>
> Diff: https://reviews.apache.org/r/69163/diff/5/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69163: Set agent and/or resource provider ID in
operation status updates.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69163/#review210962
-----------------------------------------------------------
In the final line of the commit message: s/not/no/
src/master/master.cpp
Lines 2254-2255 (patched)
<https://reviews.apache.org/r/69163/#comment295785>
It would make sense to me that we set these IDs if/when they are available. Currently I don't think it matters since the updates sent here do not need to be acknowledged; perhaps mention that in the comment?
src/slave/slave.cpp
Line 4354 (original), 4353-4359 (patched)
<https://reviews.apache.org/r/69163/#comment295790>
Why no RP ID? Here and below.
- Greg Mann
On Nov. 27, 2018, 6 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69163/
> -----------------------------------------------------------
>
> (Updated Nov. 27, 2018, 6 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 sets agent and/or resource provider ID operation status
> update messages. This is not always possible, e.g., some operations
> might fail validation so that no corresponding IDs can be extracted.
>
> Since operations failing validation are currently directly rejected by
> the master without going through a status update manager, they are not
> retried either. If a master status update manager for operations is
> introduced at a later point it should be possible to forward
> acknowledgements for updates to the master's update manager (no agent
> ID, not resource provider ID).
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 1662125ed3e47b179ee32d08c1d3af75553066ba
> src/common/protobuf_utils.cpp a45607eed4c4bae5010bcc3f3ffeabd6d911062a
> src/master/master.cpp b4b02d8b4d7d6d1aabda1f97b9bf824419f76a9e
> src/resource_provider/manager.cpp 6c81c430e9e1205d71982a7fa2bcd9aa15fc01b2
> src/resource_provider/storage/provider.cpp c137fa4f13edc58d93c03a9dd32fdf9d38b38316
> src/slave/slave.cpp 858b78620e1ef33f3587d0bd95a684917aaf5bbb
> src/tests/master_tests.cpp 651bb9ba9298fc3d7179ed86487aa55be519ce12
> src/tests/mesos.hpp 576f4bde88c069ee2fa0dd33912a034437338e7e
>
>
> Diff: https://reviews.apache.org/r/69163/diff/4/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69163: Set agent and/or resource provider ID in
operation status updates.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Dec. 1, 2018, 4:16 a.m., Greg Mann wrote:
> > src/resource_provider/manager.cpp
> > Lines 579 (patched)
> > <https://reviews.apache.org/r/69163/diff/5/?file=2111250#file2111250line579>
> >
> > Did you intend to do this in the current patch or make this a TODO?
No, this was intended to be a `TODO`.
> On Dec. 1, 2018, 4:16 a.m., Greg Mann wrote:
> > src/tests/master_tests.cpp
> > Lines 9226 (patched)
> > <https://reviews.apache.org/r/69163/diff/5/?file=2111253#file2111253line9226>
> >
> > Could you run this test in repetition to ensure that we're not introducing any flakiness?
I already did run this test in repetition and under stress during development. Are you hinting that you saw a failure, or see a problem with the test setup?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69163/#review210993
-----------------------------------------------------------
On Dec. 3, 2018, 2:20 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69163/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2018, 2:20 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 sets agent and/or resource provider ID operation status
> update messages. This is not always possible, e.g., some operations
> might fail validation so that no corresponding IDs can be extracted.
>
> Since operations failing validation are currently directly rejected by
> the master without going through a status update manager, they are not
> retried either. If a master status update manager for operations is
> introduced at a later point it should be possible to forward
> acknowledgements for updates to the master's update manager (not agent
> ID, not resource provider ID).
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 1662125ed3e47b179ee32d08c1d3af75553066ba
> src/common/protobuf_utils.cpp a45607eed4c4bae5010bcc3f3ffeabd6d911062a
> src/master/master.cpp 3b3824a67f46866cd64e32d7f9f92484b5891aa2
> src/resource_provider/manager.cpp 6c81c430e9e1205d71982a7fa2bcd9aa15fc01b2
> src/resource_provider/storage/provider.cpp a22c82c442304979fbdec0fcb74543077751a135
> src/slave/slave.cpp 858b78620e1ef33f3587d0bd95a684917aaf5bbb
> src/tests/master_tests.cpp ef2c00101fc3d30c564a9ca34884dece2cdd2651
> src/tests/mesos.hpp c08e7e6c1dbc3dd9eb980868e43368c0a423c3ee
>
>
> Diff: https://reviews.apache.org/r/69163/diff/6/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69163: Set agent and/or resource provider ID in
operation status updates.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Dec. 1, 2018, 3:16 a.m., Greg Mann wrote:
> > src/tests/master_tests.cpp
> > Lines 9226 (patched)
> > <https://reviews.apache.org/r/69163/diff/5/?file=2111253#file2111253line9226>
> >
> > Could you run this test in repetition to ensure that we're not introducing any flakiness?
>
> Benjamin Bannier wrote:
> I already did run this test in repetition and under stress during development. Are you hinting that you saw a failure, or see a problem with the test setup?
Nope I hadn't noticed anything - since only `make check` was listed in the "Testing Done" field, I assumed that it hadn't been run in repetition. As long as you've done that, I'm satisfied! Thanks :)
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69163/#review210993
-----------------------------------------------------------
On Dec. 3, 2018, 1:20 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69163/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2018, 1:20 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 sets agent and/or resource provider ID operation status
> update messages. This is not always possible, e.g., some operations
> might fail validation so that no corresponding IDs can be extracted.
>
> Since operations failing validation are currently directly rejected by
> the master without going through a status update manager, they are not
> retried either. If a master status update manager for operations is
> introduced at a later point it should be possible to forward
> acknowledgements for updates to the master's update manager (not agent
> ID, not resource provider ID).
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 1662125ed3e47b179ee32d08c1d3af75553066ba
> src/common/protobuf_utils.cpp a45607eed4c4bae5010bcc3f3ffeabd6d911062a
> src/master/master.cpp 3b3824a67f46866cd64e32d7f9f92484b5891aa2
> src/resource_provider/manager.cpp 6c81c430e9e1205d71982a7fa2bcd9aa15fc01b2
> src/resource_provider/storage/provider.cpp a22c82c442304979fbdec0fcb74543077751a135
> src/slave/slave.cpp 858b78620e1ef33f3587d0bd95a684917aaf5bbb
> src/tests/master_tests.cpp ef2c00101fc3d30c564a9ca34884dece2cdd2651
> src/tests/mesos.hpp c08e7e6c1dbc3dd9eb980868e43368c0a423c3ee
>
>
> Diff: https://reviews.apache.org/r/69163/diff/6/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69163: Set agent and/or resource provider ID in
operation status updates.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69163/#review210993
-----------------------------------------------------------
src/resource_provider/manager.cpp
Lines 579 (patched)
<https://reviews.apache.org/r/69163/#comment295834>
Did you intend to do this in the current patch or make this a TODO?
src/tests/master_tests.cpp
Lines 9226 (patched)
<https://reviews.apache.org/r/69163/#comment295831>
Could you run this test in repetition to ensure that we're not introducing any flakiness?
src/tests/master_tests.cpp
Lines 9271-9300 (patched)
<https://reviews.apache.org/r/69163/#comment295830>
This could be made more concise with the following pattern:
```
EXPECT_CALL(*scheduler, connected(_))
.WillOnce(v1::scheduler::SendSubscribe(v1::DEFAULT_FRAMEWORK_INFO));
Future<Event::Subscribed> subscribed;
EXPECT_CALL(*scheduler, subscribed(_, _))
.WillOnce(FutureArg<1>(&subscribed));
```
src/tests/master_tests.cpp
Lines 9351-9352 (patched)
<https://reviews.apache.org/r/69163/#comment295832>
Seems like we might as well also verify that these fields contain the correct values?
src/tests/mesos.hpp
Lines 3165-3166 (patched)
<https://reviews.apache.org/r/69163/#comment295833>
Sorry I don't understand this comment - could you clarify?
- 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/69163/
> -----------------------------------------------------------
>
> (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 sets agent and/or resource provider ID operation status
> update messages. This is not always possible, e.g., some operations
> might fail validation so that no corresponding IDs can be extracted.
>
> Since operations failing validation are currently directly rejected by
> the master without going through a status update manager, they are not
> retried either. If a master status update manager for operations is
> introduced at a later point it should be possible to forward
> acknowledgements for updates to the master's update manager (not agent
> ID, not resource provider ID).
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 1662125ed3e47b179ee32d08c1d3af75553066ba
> src/common/protobuf_utils.cpp a45607eed4c4bae5010bcc3f3ffeabd6d911062a
> src/master/master.cpp 3b3824a67f46866cd64e32d7f9f92484b5891aa2
> src/resource_provider/manager.cpp 6c81c430e9e1205d71982a7fa2bcd9aa15fc01b2
> src/resource_provider/storage/provider.cpp a22c82c442304979fbdec0fcb74543077751a135
> src/slave/slave.cpp 858b78620e1ef33f3587d0bd95a684917aaf5bbb
> src/tests/master_tests.cpp ef2c00101fc3d30c564a9ca34884dece2cdd2651
> src/tests/mesos.hpp c08e7e6c1dbc3dd9eb980868e43368c0a423c3ee
>
>
> Diff: https://reviews.apache.org/r/69163/diff/5/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69163: Set agent and/or resource provider ID in
operation status updates.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69163/#review211060
-----------------------------------------------------------
src/resource_provider/storage/provider.cpp
Lines 3093 (patched)
<https://reviews.apache.org/r/69163/#comment295918>
s/info/operation/
- Chun-Hung Hsiao
On Dec. 3, 2018, 1:20 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69163/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2018, 1:20 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 sets agent and/or resource provider ID operation status
> update messages. This is not always possible, e.g., some operations
> might fail validation so that no corresponding IDs can be extracted.
>
> Since operations failing validation are currently directly rejected by
> the master without going through a status update manager, they are not
> retried either. If a master status update manager for operations is
> introduced at a later point it should be possible to forward
> acknowledgements for updates to the master's update manager.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 1662125ed3e47b179ee32d08c1d3af75553066ba
> src/common/protobuf_utils.cpp a45607eed4c4bae5010bcc3f3ffeabd6d911062a
> src/master/master.cpp 3b3824a67f46866cd64e32d7f9f92484b5891aa2
> src/resource_provider/manager.cpp 6c81c430e9e1205d71982a7fa2bcd9aa15fc01b2
> src/resource_provider/storage/provider.cpp a22c82c442304979fbdec0fcb74543077751a135
> src/slave/slave.cpp 858b78620e1ef33f3587d0bd95a684917aaf5bbb
> src/tests/master_tests.cpp ef2c00101fc3d30c564a9ca34884dece2cdd2651
> src/tests/mesos.hpp c08e7e6c1dbc3dd9eb980868e43368c0a423c3ee
>
>
> Diff: https://reviews.apache.org/r/69163/diff/6/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69163: Set agent and/or resource provider ID in
operation status updates.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Dec. 4, 2018, 6:36 a.m., Chun-Hung Hsiao wrote:
> > src/tests/master_tests.cpp
> > Lines 9276 (patched)
> > <https://reviews.apache.org/r/69163/diff/6/?file=2111727#file2111727line9276>
> >
> > Will we hit MESOS-6033 here?
> >
> > Also, will `disconnected` be called during teardown? If yes maybe we want to expect it to be called at most once?
Re:`disconnected`, I don't see how it could be called. The driver will be destructed before the scheduler which would terminate its `MesosProcess`, i.e., the scheduler lives longer than the entity invoking its methods.
Re:MESOS-6033, I am not sure. I follow the usual pattern for v1 schedulers, though, so I'd expect this issue to require a bigger cleanup.
Dropping this for now, please feel free to reopen if I am missing something.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69163/#review211010
-----------------------------------------------------------
On Dec. 3, 2018, 2:20 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69163/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2018, 2:20 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 sets agent and/or resource provider ID operation status
> update messages. This is not always possible, e.g., some operations
> might fail validation so that no corresponding IDs can be extracted.
>
> Since operations failing validation are currently directly rejected by
> the master without going through a status update manager, they are not
> retried either. If a master status update manager for operations is
> introduced at a later point it should be possible to forward
> acknowledgements for updates to the master's update manager (not agent
> ID, not resource provider ID).
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 1662125ed3e47b179ee32d08c1d3af75553066ba
> src/common/protobuf_utils.cpp a45607eed4c4bae5010bcc3f3ffeabd6d911062a
> src/master/master.cpp 3b3824a67f46866cd64e32d7f9f92484b5891aa2
> src/resource_provider/manager.cpp 6c81c430e9e1205d71982a7fa2bcd9aa15fc01b2
> src/resource_provider/storage/provider.cpp a22c82c442304979fbdec0fcb74543077751a135
> src/slave/slave.cpp 858b78620e1ef33f3587d0bd95a684917aaf5bbb
> src/tests/master_tests.cpp ef2c00101fc3d30c564a9ca34884dece2cdd2651
> src/tests/mesos.hpp c08e7e6c1dbc3dd9eb980868e43368c0a423c3ee
>
>
> Diff: https://reviews.apache.org/r/69163/diff/6/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69163: Set agent and/or resource provider ID in
operation status updates.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On Dec. 4, 2018, 5:36 a.m., Chun-Hung Hsiao wrote:
> > src/tests/master_tests.cpp
> > Lines 9276 (patched)
> > <https://reviews.apache.org/r/69163/diff/6/?file=2111727#file2111727line9276>
> >
> > Will we hit MESOS-6033 here?
> >
> > Also, will `disconnected` be called during teardown? If yes maybe we want to expect it to be called at most once?
>
> Benjamin Bannier wrote:
> Re:`disconnected`, I don't see how it could be called. The driver will be destructed before the scheduler which would terminate its `MesosProcess`, i.e., the scheduler lives longer than the entity invoking its methods.
>
> Re:MESOS-6033, I am not sure. I follow the usual pattern for v1 schedulers, though, so I'd expect this issue to require a bigger cleanup.
>
> Dropping this for now, please feel free to reopen if I am missing something.
Thanks for looking into this. If `disconnected` won't be called, then it seems to me that we'll probably not hit MESOS-6033.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69163/#review211010
-----------------------------------------------------------
On Dec. 3, 2018, 1:20 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69163/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2018, 1:20 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 sets agent and/or resource provider ID operation status
> update messages. This is not always possible, e.g., some operations
> might fail validation so that no corresponding IDs can be extracted.
>
> Since operations failing validation are currently directly rejected by
> the master without going through a status update manager, they are not
> retried either. If a master status update manager for operations is
> introduced at a later point it should be possible to forward
> acknowledgements for updates to the master's update manager (not agent
> ID, not resource provider ID).
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 1662125ed3e47b179ee32d08c1d3af75553066ba
> src/common/protobuf_utils.cpp a45607eed4c4bae5010bcc3f3ffeabd6d911062a
> src/master/master.cpp 3b3824a67f46866cd64e32d7f9f92484b5891aa2
> src/resource_provider/manager.cpp 6c81c430e9e1205d71982a7fa2bcd9aa15fc01b2
> src/resource_provider/storage/provider.cpp a22c82c442304979fbdec0fcb74543077751a135
> src/slave/slave.cpp 858b78620e1ef33f3587d0bd95a684917aaf5bbb
> src/tests/master_tests.cpp ef2c00101fc3d30c564a9ca34884dece2cdd2651
> src/tests/mesos.hpp c08e7e6c1dbc3dd9eb980868e43368c0a423c3ee
>
>
> Diff: https://reviews.apache.org/r/69163/diff/6/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69163: Set agent and/or resource provider ID in
operation status updates.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69163/#review211010
-----------------------------------------------------------
src/tests/master_tests.cpp
Lines 9276 (patched)
<https://reviews.apache.org/r/69163/#comment295863>
Will we hit MESOS-6033 here?
Also, will `disconnected` be called during teardown? If yes maybe we want to expect it to be called at most once?
- Chun-Hung Hsiao
On Dec. 3, 2018, 1:20 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69163/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2018, 1:20 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 sets agent and/or resource provider ID operation status
> update messages. This is not always possible, e.g., some operations
> might fail validation so that no corresponding IDs can be extracted.
>
> Since operations failing validation are currently directly rejected by
> the master without going through a status update manager, they are not
> retried either. If a master status update manager for operations is
> introduced at a later point it should be possible to forward
> acknowledgements for updates to the master's update manager (not agent
> ID, not resource provider ID).
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 1662125ed3e47b179ee32d08c1d3af75553066ba
> src/common/protobuf_utils.cpp a45607eed4c4bae5010bcc3f3ffeabd6d911062a
> src/master/master.cpp 3b3824a67f46866cd64e32d7f9f92484b5891aa2
> src/resource_provider/manager.cpp 6c81c430e9e1205d71982a7fa2bcd9aa15fc01b2
> src/resource_provider/storage/provider.cpp a22c82c442304979fbdec0fcb74543077751a135
> src/slave/slave.cpp 858b78620e1ef33f3587d0bd95a684917aaf5bbb
> src/tests/master_tests.cpp ef2c00101fc3d30c564a9ca34884dece2cdd2651
> src/tests/mesos.hpp c08e7e6c1dbc3dd9eb980868e43368c0a423c3ee
>
>
> Diff: https://reviews.apache.org/r/69163/diff/6/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69163: Set agent and/or resource provider ID in
operation status updates.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69163/#review211034
-----------------------------------------------------------
Ship it!
IMO you could get rid of the last bit in the commit message which says "(not agent ID, not resource provider ID)".
- Greg Mann
On Dec. 3, 2018, 1:20 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69163/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2018, 1:20 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 sets agent and/or resource provider ID operation status
> update messages. This is not always possible, e.g., some operations
> might fail validation so that no corresponding IDs can be extracted.
>
> Since operations failing validation are currently directly rejected by
> the master without going through a status update manager, they are not
> retried either. If a master status update manager for operations is
> introduced at a later point it should be possible to forward
> acknowledgements for updates to the master's update manager (not agent
> ID, not resource provider ID).
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 1662125ed3e47b179ee32d08c1d3af75553066ba
> src/common/protobuf_utils.cpp a45607eed4c4bae5010bcc3f3ffeabd6d911062a
> src/master/master.cpp 3b3824a67f46866cd64e32d7f9f92484b5891aa2
> src/resource_provider/manager.cpp 6c81c430e9e1205d71982a7fa2bcd9aa15fc01b2
> src/resource_provider/storage/provider.cpp a22c82c442304979fbdec0fcb74543077751a135
> src/slave/slave.cpp 858b78620e1ef33f3587d0bd95a684917aaf5bbb
> src/tests/master_tests.cpp ef2c00101fc3d30c564a9ca34884dece2cdd2651
> src/tests/mesos.hpp c08e7e6c1dbc3dd9eb980868e43368c0a423c3ee
>
>
> Diff: https://reviews.apache.org/r/69163/diff/6/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69163: Set agent and/or resource provider ID in
operation status updates.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69163/
-----------------------------------------------------------
(Updated Dec. 3, 2018, 2:20 p.m.)
Review request for mesos, Chun-Hung Hsiao, Gastón Kleiman, and James DeFelice.
Changes
-------
Addressed issues raised by Greg.
Bugs: MESOS-9293
https://issues.apache.org/jira/browse/MESOS-9293
Repository: mesos
Description
-------
This patch sets agent and/or resource provider ID operation status
update messages. This is not always possible, e.g., some operations
might fail validation so that no corresponding IDs can be extracted.
Since operations failing validation are currently directly rejected by
the master without going through a status update manager, they are not
retried either. If a master status update manager for operations is
introduced at a later point it should be possible to forward
acknowledgements for updates to the master's update manager (not agent
ID, not resource provider ID).
Diffs (updated)
-----
src/common/protobuf_utils.hpp 1662125ed3e47b179ee32d08c1d3af75553066ba
src/common/protobuf_utils.cpp a45607eed4c4bae5010bcc3f3ffeabd6d911062a
src/master/master.cpp 3b3824a67f46866cd64e32d7f9f92484b5891aa2
src/resource_provider/manager.cpp 6c81c430e9e1205d71982a7fa2bcd9aa15fc01b2
src/resource_provider/storage/provider.cpp a22c82c442304979fbdec0fcb74543077751a135
src/slave/slave.cpp 858b78620e1ef33f3587d0bd95a684917aaf5bbb
src/tests/master_tests.cpp ef2c00101fc3d30c564a9ca34884dece2cdd2651
src/tests/mesos.hpp c08e7e6c1dbc3dd9eb980868e43368c0a423c3ee
Diff: https://reviews.apache.org/r/69163/diff/6/
Changes: https://reviews.apache.org/r/69163/diff/5-6/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 69163: Set agent and/or resource provider ID in
operation status updates.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69163/
-----------------------------------------------------------
(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 sets agent and/or resource provider ID operation status
update messages. This is not always possible, e.g., some operations
might fail validation so that no corresponding IDs can be extracted.
Since operations failing validation are currently directly rejected by
the master without going through a status update manager, they are not
retried either. If a master status update manager for operations is
introduced at a later point it should be possible to forward
acknowledgements for updates to the master's update manager (not agent
ID, not resource provider ID).
Diffs (updated)
-----
src/common/protobuf_utils.hpp 1662125ed3e47b179ee32d08c1d3af75553066ba
src/common/protobuf_utils.cpp a45607eed4c4bae5010bcc3f3ffeabd6d911062a
src/master/master.cpp 3b3824a67f46866cd64e32d7f9f92484b5891aa2
src/resource_provider/manager.cpp 6c81c430e9e1205d71982a7fa2bcd9aa15fc01b2
src/resource_provider/storage/provider.cpp a22c82c442304979fbdec0fcb74543077751a135
src/slave/slave.cpp 858b78620e1ef33f3587d0bd95a684917aaf5bbb
src/tests/master_tests.cpp ef2c00101fc3d30c564a9ca34884dece2cdd2651
src/tests/mesos.hpp c08e7e6c1dbc3dd9eb980868e43368c0a423c3ee
Diff: https://reviews.apache.org/r/69163/diff/5/
Changes: https://reviews.apache.org/r/69163/diff/4-5/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 69163: Set agent and/or resource provider ID in
operation status updates.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69163/
-----------------------------------------------------------
(Updated Nov. 27, 2018, 7 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 sets agent and/or resource provider ID operation status
update messages. This is not always possible, e.g., some operations
might fail validation so that no corresponding IDs can be extracted.
Since operations failing validation are currently directly rejected by
the master without going through a status update manager, they are not
retried either. If a master status update manager for operations is
introduced at a later point it should be possible to forward
acknowledgements for updates to the master's update manager (no agent
ID, not resource provider ID).
Diffs (updated)
-----
src/common/protobuf_utils.hpp 1662125ed3e47b179ee32d08c1d3af75553066ba
src/common/protobuf_utils.cpp a45607eed4c4bae5010bcc3f3ffeabd6d911062a
src/master/master.cpp b4b02d8b4d7d6d1aabda1f97b9bf824419f76a9e
src/resource_provider/manager.cpp 6c81c430e9e1205d71982a7fa2bcd9aa15fc01b2
src/resource_provider/storage/provider.cpp c137fa4f13edc58d93c03a9dd32fdf9d38b38316
src/slave/slave.cpp 858b78620e1ef33f3587d0bd95a684917aaf5bbb
src/tests/master_tests.cpp 651bb9ba9298fc3d7179ed86487aa55be519ce12
src/tests/mesos.hpp 576f4bde88c069ee2fa0dd33912a034437338e7e
Diff: https://reviews.apache.org/r/69163/diff/4/
Changes: https://reviews.apache.org/r/69163/diff/3-4/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 69163: Set agent and/or resource provider ID in
operation status updates.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69163/
-----------------------------------------------------------
(Updated Nov. 12, 2018, 9:49 p.m.)
Review request for mesos, Chun-Hung Hsiao, Gastón Kleiman, and James DeFelice.
Changes
-------
Rebased; added dedicated test.
Bugs: MESOS-9293
https://issues.apache.org/jira/browse/MESOS-9293
Repository: mesos
Description
-------
This patch sets agent and/or resource provider ID operation status
update messages. This is not always possible, e.g., some operations
might fail validation so that no corresponding IDs can be extracted.
Since operations failing validation are currently directly rejected by
the master without going through a status update manager, they are not
retried either. If a master status update manager for operations is
introduced at a later point it should be possible to forward
acknowledgements for updates to the master's update manager (no agent
ID, not resource provider ID).
Diffs (updated)
-----
src/common/protobuf_utils.hpp 1662125ed3e47b179ee32d08c1d3af75553066ba
src/common/protobuf_utils.cpp a45607eed4c4bae5010bcc3f3ffeabd6d911062a
src/master/master.cpp 1e326ec42a7f79a0835529a4655e7ec272f1cf40
src/resource_provider/manager.cpp 6c81c430e9e1205d71982a7fa2bcd9aa15fc01b2
src/resource_provider/storage/provider.cpp c137fa4f13edc58d93c03a9dd32fdf9d38b38316
src/slave/slave.cpp 74f6fb9036a9ac4f587f53ec2df04eeb4c167bfb
src/tests/master_tests.cpp ac6bf379c5906cf9612284911c121c9457f648a0
Diff: https://reviews.apache.org/r/69163/diff/3/
Changes: https://reviews.apache.org/r/69163/diff/2-3/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 69163: Set agent and/or resource provider ID in
operation status updates.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69163/#review210227
-----------------------------------------------------------
PASS: Mesos patch 69163 was successfully built and tested.
Reviews applied: `['69159', '69160', '68147', '69157', '69158', '69161', '69162', '69163']`
All the build artifacts available at: http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2557/mesos-review-69163
- Mesos Reviewbot Windows
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/69163/
> -----------------------------------------------------------
>
> (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 sets agent and/or resource provider ID operation status
> update messages. This is not always possible, e.g., some operations
> might fail validation so that no corresponding IDs can be extracted.
>
> Since operations failing validation are currently directly rejected by
> the master without going through a status update manager, they are not
> retried either. If a master status update manager for operations is
> introduced at a later point it should be possible to forward
> acknowledgements for updates to the master's update manager (no agent
> ID, not resource provider ID).
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 1662125ed3e47b179ee32d08c1d3af75553066ba
> src/common/protobuf_utils.cpp 2ae37ac610894652a0214022a0cf04c4365396dd
> src/master/master.cpp f88c7c1f03f0de7236aad9e3bf4bfac82e91bc65
> src/resource_provider/storage/provider.cpp db783b53558811081fb2671e005e8bbbd9edbede
> src/slave/slave.cpp 74f6fb9036a9ac4f587f53ec2df04eeb4c167bfb
> src/tests/slave_tests.cpp 637bedce20f63dfc1702ef549bcea8f790477be3
>
>
> Diff: https://reviews.apache.org/r/69163/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69163: Set agent and/or resource provider ID in
operation status updates.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69163/#review210034
-----------------------------------------------------------
PASS: Mesos patch 69163 was successfully built and tested.
Reviews applied: `['68147', '69157', '69158', '69159', '69160', '69161', '69162', '69163']`
All the build artifacts available at: http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2529/mesos-review-69163
- Mesos Reviewbot Windows
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/69163/
> -----------------------------------------------------------
>
> (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 sets agent and/or resource provider ID operation status
> update messages. This is not always possible, e.g., some operations
> might fail validation so that no corresponding IDs can be extracted.
>
> Since operations failing validation are currently directly rejected by
> the master without going through a status update manager, they are not
> retried either. If a master status update manager for operations is
> introduced at a later point it should be possible to forward
> acknowledgements for updates to the master's update manager (no agent
> ID, not resource provider ID).
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 1662125ed3e47b179ee32d08c1d3af75553066ba
> src/common/protobuf_utils.cpp 77139d8a3931dc1e9e00fcea83d77d96244a34f3
> src/master/master.cpp 0c95c438975efd949cbf86f7d8bfea940c20a43a
> src/resource_provider/storage/provider.cpp db783b53558811081fb2671e005e8bbbd9edbede
> src/slave/slave.cpp 7bb2b291a7b67563adf88bcedca1392890f17ffe
> src/tests/slave_tests.cpp 637bedce20f63dfc1702ef549bcea8f790477be3
>
>
> Diff: https://reviews.apache.org/r/69163/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69163: Set agent and/or resource provider ID in
operation status updates.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69163/#review210050
-----------------------------------------------------------
Patch looks great!
Reviews applied: [68147, 69157, 69158, 69159, 69160, 69161, 69162, 69163]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Oct. 25, 2018, 3:54 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69163/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2018, 3: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 sets agent and/or resource provider ID operation status
> update messages. This is not always possible, e.g., some operations
> might fail validation so that no corresponding IDs can be extracted.
>
> Since operations failing validation are currently directly rejected by
> the master without going through a status update manager, they are not
> retried either. If a master status update manager for operations is
> introduced at a later point it should be possible to forward
> acknowledgements for updates to the master's update manager (no agent
> ID, not resource provider ID).
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 1662125ed3e47b179ee32d08c1d3af75553066ba
> src/common/protobuf_utils.cpp 77139d8a3931dc1e9e00fcea83d77d96244a34f3
> src/master/master.cpp 0c95c438975efd949cbf86f7d8bfea940c20a43a
> src/resource_provider/storage/provider.cpp db783b53558811081fb2671e005e8bbbd9edbede
> src/slave/slave.cpp 7bb2b291a7b67563adf88bcedca1392890f17ffe
> src/tests/slave_tests.cpp 637bedce20f63dfc1702ef549bcea8f790477be3
>
>
> Diff: https://reviews.apache.org/r/69163/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>