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/08/15 13:53:01 UTC
Review Request 68362: Made RP manager only send resource provider ID
on state updates.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68362/
-----------------------------------------------------------
Review request for mesos and Chun-Hung Hsiao.
Bugs: MESOS-8403
https://issues.apache.org/jira/browse/MESOS-8403
Repository: mesos
Description
-------
With the introduction of the resource provider `SUBSCRIBE` event which
contains the full `ResourceProviderInfo` of a subscribed resource
provider, we can avoid sending the `ResourceProviderInfo` in
`UPDATE_STATE` events and instead only send a `ResourceProviderID`
like in most other messages.
Diffs
-----
src/resource_provider/manager.cpp abd7e38e5517ea600f9fc9b8a96c7d0d26df0620
src/resource_provider/message.hpp 9307f8859035dfafe0952e9026b078b44121537e
src/slave/slave.cpp 78e8666f402be58af5b6e20a715da4998af2615c
src/tests/resource_provider_manager_tests.cpp 0b9e985cf3b4ded8590615b418e52a2a11f1c1aa
Diff: https://reviews.apache.org/r/68362/diff/1/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 68362: Made RP manager only send resource provider
ID on state updates.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68362/#review207627
-----------------------------------------------------------
Ship it!
Ship It!
- Chun-Hung Hsiao
On Aug. 15, 2018, 1:53 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68362/
> -----------------------------------------------------------
>
> (Updated Aug. 15, 2018, 1:53 p.m.)
>
>
> Review request for mesos and Chun-Hung Hsiao.
>
>
> Bugs: MESOS-8403
> https://issues.apache.org/jira/browse/MESOS-8403
>
>
> Repository: mesos
>
>
> Description
> -------
>
> With the introduction of the resource provider `SUBSCRIBE` event which
> contains the full `ResourceProviderInfo` of a subscribed resource
> provider, we can avoid sending the `ResourceProviderInfo` in
> `UPDATE_STATE` events and instead only send a `ResourceProviderID`
> like in most other messages.
>
>
> Diffs
> -----
>
> src/resource_provider/manager.cpp abd7e38e5517ea600f9fc9b8a96c7d0d26df0620
> src/resource_provider/message.hpp 9307f8859035dfafe0952e9026b078b44121537e
> src/slave/slave.cpp 679394a549cdfe84d64a102164c8652ad96f1eb2
> src/tests/resource_provider_manager_tests.cpp 0b9e985cf3b4ded8590615b418e52a2a11f1c1aa
>
>
> Diff: https://reviews.apache.org/r/68362/diff/5/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 68362: Made RP manager only send resource provider
ID on state updates.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Aug. 18, 2018, 12:34 a.m., Chun-Hung Hsiao wrote:
> > src/slave/slave.cpp
> > Lines 7684-7690 (original)
> > <https://reviews.apache.org/r/68362/diff/4/?file=2074583#file2074583line7684>
> >
> > Where do we update the operations and total resources?
The `if` statement removed here was for the case where a resource provider was unknown when the `UPDATE_STATE` was received. This is not possible anymore as we always inform the master a resource provider subscribes.
The code handling _updates_ to total resource or operations was in the `else` block previously which will be always executed now.
Dropping.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68362/#review207565
-----------------------------------------------------------
On Aug. 15, 2018, 3:53 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68362/
> -----------------------------------------------------------
>
> (Updated Aug. 15, 2018, 3:53 p.m.)
>
>
> Review request for mesos and Chun-Hung Hsiao.
>
>
> Bugs: MESOS-8403
> https://issues.apache.org/jira/browse/MESOS-8403
>
>
> Repository: mesos
>
>
> Description
> -------
>
> With the introduction of the resource provider `SUBSCRIBE` event which
> contains the full `ResourceProviderInfo` of a subscribed resource
> provider, we can avoid sending the `ResourceProviderInfo` in
> `UPDATE_STATE` events and instead only send a `ResourceProviderID`
> like in most other messages.
>
>
> Diffs
> -----
>
> src/resource_provider/manager.cpp abd7e38e5517ea600f9fc9b8a96c7d0d26df0620
> src/resource_provider/message.hpp 9307f8859035dfafe0952e9026b078b44121537e
> src/slave/slave.cpp 679394a549cdfe84d64a102164c8652ad96f1eb2
> src/tests/resource_provider_manager_tests.cpp 0b9e985cf3b4ded8590615b418e52a2a11f1c1aa
>
>
> Diff: https://reviews.apache.org/r/68362/diff/5/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 68362: Made RP manager only send resource provider
ID on state updates.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68362/#review207565
-----------------------------------------------------------
src/slave/slave.cpp
Lines 7684-7690 (original)
<https://reviews.apache.org/r/68362/#comment290965>
Where do we update the operations and total resources?
- Chun-Hung Hsiao
On Aug. 15, 2018, 1:53 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68362/
> -----------------------------------------------------------
>
> (Updated Aug. 15, 2018, 1:53 p.m.)
>
>
> Review request for mesos and Chun-Hung Hsiao.
>
>
> Bugs: MESOS-8403
> https://issues.apache.org/jira/browse/MESOS-8403
>
>
> Repository: mesos
>
>
> Description
> -------
>
> With the introduction of the resource provider `SUBSCRIBE` event which
> contains the full `ResourceProviderInfo` of a subscribed resource
> provider, we can avoid sending the `ResourceProviderInfo` in
> `UPDATE_STATE` events and instead only send a `ResourceProviderID`
> like in most other messages.
>
>
> Diffs
> -----
>
> src/resource_provider/manager.cpp abd7e38e5517ea600f9fc9b8a96c7d0d26df0620
> src/resource_provider/message.hpp 9307f8859035dfafe0952e9026b078b44121537e
> src/slave/slave.cpp 78e8666f402be58af5b6e20a715da4998af2615c
> src/tests/resource_provider_manager_tests.cpp 0b9e985cf3b4ded8590615b418e52a2a11f1c1aa
>
>
> Diff: https://reviews.apache.org/r/68362/diff/4/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 68362: Made RP manager only send resource provider
ID on state updates.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Aug. 16, 2018, 12:14 a.m., Chun-Hung Hsiao wrote:
> > src/slave/slave.cpp
> > Line 7653 (original), 7648 (patched)
> > <https://reviews.apache.org/r/68362/diff/1/?file=2073015#file2073015line7653>
> >
> > This will create a resource provider without a resource version, which in turn makes `Slave::generateResourceProviderUpdate()` create an `UpdateSlaveMessage` missing the required field `resource_version_uuid`.
I don't see where this generates an `UpdateSlaveMessage`. Are you sure we'll send one?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68362/#review207363
-----------------------------------------------------------
On Aug. 15, 2018, 3:53 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68362/
> -----------------------------------------------------------
>
> (Updated Aug. 15, 2018, 3:53 p.m.)
>
>
> Review request for mesos and Chun-Hung Hsiao.
>
>
> Bugs: MESOS-8403
> https://issues.apache.org/jira/browse/MESOS-8403
>
>
> Repository: mesos
>
>
> Description
> -------
>
> With the introduction of the resource provider `SUBSCRIBE` event which
> contains the full `ResourceProviderInfo` of a subscribed resource
> provider, we can avoid sending the `ResourceProviderInfo` in
> `UPDATE_STATE` events and instead only send a `ResourceProviderID`
> like in most other messages.
>
>
> Diffs
> -----
>
> src/resource_provider/manager.cpp abd7e38e5517ea600f9fc9b8a96c7d0d26df0620
> src/resource_provider/message.hpp 9307f8859035dfafe0952e9026b078b44121537e
> src/slave/slave.cpp 78e8666f402be58af5b6e20a715da4998af2615c
> src/tests/resource_provider_manager_tests.cpp 0b9e985cf3b4ded8590615b418e52a2a11f1c1aa
>
>
> Diff: https://reviews.apache.org/r/68362/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 68362: Made RP manager only send resource provider
ID on state updates.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On Aug. 15, 2018, 10:14 p.m., Chun-Hung Hsiao wrote:
> > src/slave/slave.cpp
> > Line 7653 (original), 7648 (patched)
> > <https://reviews.apache.org/r/68362/diff/1/?file=2073015#file2073015line7653>
> >
> > This will create a resource provider without a resource version, which in turn makes `Slave::generateResourceProviderUpdate()` create an `UpdateSlaveMessage` missing the required field `resource_version_uuid`.
>
> Benjamin Bannier wrote:
> I don't see where this generates an `UpdateSlaveMessage`. Are you sure we'll send one?
I should have pointed out the failed test: `ResourceProviderManagerHttpApiTest.ResourceProviderSubscribeDisconnect`. IIRC disconnection would trigger an `UpdateSlaveMessage`.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68362/#review207363
-----------------------------------------------------------
On Aug. 15, 2018, 1:53 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68362/
> -----------------------------------------------------------
>
> (Updated Aug. 15, 2018, 1:53 p.m.)
>
>
> Review request for mesos and Chun-Hung Hsiao.
>
>
> Bugs: MESOS-8403
> https://issues.apache.org/jira/browse/MESOS-8403
>
>
> Repository: mesos
>
>
> Description
> -------
>
> With the introduction of the resource provider `SUBSCRIBE` event which
> contains the full `ResourceProviderInfo` of a subscribed resource
> provider, we can avoid sending the `ResourceProviderInfo` in
> `UPDATE_STATE` events and instead only send a `ResourceProviderID`
> like in most other messages.
>
>
> Diffs
> -----
>
> src/resource_provider/manager.cpp abd7e38e5517ea600f9fc9b8a96c7d0d26df0620
> src/resource_provider/message.hpp 9307f8859035dfafe0952e9026b078b44121537e
> src/slave/slave.cpp 78e8666f402be58af5b6e20a715da4998af2615c
> src/tests/resource_provider_manager_tests.cpp 0b9e985cf3b4ded8590615b418e52a2a11f1c1aa
>
>
> Diff: https://reviews.apache.org/r/68362/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 68362: Made RP manager only send resource provider
ID on state updates.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Aug. 16, 2018, 12:14 a.m., Chun-Hung Hsiao wrote:
> > src/slave/slave.cpp
> > Line 7653 (original), 7648 (patched)
> > <https://reviews.apache.org/r/68362/diff/1/?file=2073015#file2073015line7653>
> >
> > This will create a resource provider without a resource version, which in turn makes `Slave::generateResourceProviderUpdate()` create an `UpdateSlaveMessage` missing the required field `resource_version_uuid`.
>
> Benjamin Bannier wrote:
> I don't see where this generates an `UpdateSlaveMessage`. Are you sure we'll send one?
>
> Chun-Hung Hsiao wrote:
> I should have pointed out the failed test: `ResourceProviderManagerHttpApiTest.ResourceProviderSubscribeDisconnect`. IIRC disconnection would trigger an `UpdateSlaveMessage`.
Urg, that's not good. I added https://reviews.apache.org/r/68407/ to this chain and made this patch depend on it.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68362/#review207363
-----------------------------------------------------------
On Aug. 15, 2018, 3:53 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68362/
> -----------------------------------------------------------
>
> (Updated Aug. 15, 2018, 3:53 p.m.)
>
>
> Review request for mesos and Chun-Hung Hsiao.
>
>
> Bugs: MESOS-8403
> https://issues.apache.org/jira/browse/MESOS-8403
>
>
> Repository: mesos
>
>
> Description
> -------
>
> With the introduction of the resource provider `SUBSCRIBE` event which
> contains the full `ResourceProviderInfo` of a subscribed resource
> provider, we can avoid sending the `ResourceProviderInfo` in
> `UPDATE_STATE` events and instead only send a `ResourceProviderID`
> like in most other messages.
>
>
> Diffs
> -----
>
> src/resource_provider/manager.cpp abd7e38e5517ea600f9fc9b8a96c7d0d26df0620
> src/resource_provider/message.hpp 9307f8859035dfafe0952e9026b078b44121537e
> src/slave/slave.cpp 78e8666f402be58af5b6e20a715da4998af2615c
> src/tests/resource_provider_manager_tests.cpp 0b9e985cf3b4ded8590615b418e52a2a11f1c1aa
>
>
> Diff: https://reviews.apache.org/r/68362/diff/4/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 68362: Made RP manager only send resource provider
ID on state updates.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68362/#review207363
-----------------------------------------------------------
src/slave/slave.cpp
Line 7653 (original), 7648 (patched)
<https://reviews.apache.org/r/68362/#comment290731>
This will create a resource provider without a resource version, which in turn makes `Slave::generateResourceProviderUpdate()` create an `UpdateSlaveMessage` missing the required field `resource_version_uuid`.
- Chun-Hung Hsiao
On Aug. 15, 2018, 1:53 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68362/
> -----------------------------------------------------------
>
> (Updated Aug. 15, 2018, 1:53 p.m.)
>
>
> Review request for mesos and Chun-Hung Hsiao.
>
>
> Bugs: MESOS-8403
> https://issues.apache.org/jira/browse/MESOS-8403
>
>
> Repository: mesos
>
>
> Description
> -------
>
> With the introduction of the resource provider `SUBSCRIBE` event which
> contains the full `ResourceProviderInfo` of a subscribed resource
> provider, we can avoid sending the `ResourceProviderInfo` in
> `UPDATE_STATE` events and instead only send a `ResourceProviderID`
> like in most other messages.
>
>
> Diffs
> -----
>
> src/resource_provider/manager.cpp abd7e38e5517ea600f9fc9b8a96c7d0d26df0620
> src/resource_provider/message.hpp 9307f8859035dfafe0952e9026b078b44121537e
> src/slave/slave.cpp 78e8666f402be58af5b6e20a715da4998af2615c
> src/tests/resource_provider_manager_tests.cpp 0b9e985cf3b4ded8590615b418e52a2a11f1c1aa
>
>
> Diff: https://reviews.apache.org/r/68362/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>