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 2017/07/27 14:49:46 UTC
Review Request 61181: Stored subscribed resources in resource provider
manager.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61181/
-----------------------------------------------------------
Review request for mesos, Jie Yu and Jan Schlicht.
Bugs: MESOS-7837
https://issues.apache.org/jira/browse/MESOS-7837
Repository: mesos
Description
-------
In order to be able to always communicate the aggregated total
resources available on all subscribed resource providers, a resource
provider manager needs to keep track of the resources of all
subscribed resource providers. This commit adds a field for that the
manager's internal data structures for that purpose.
To make assigned 'ResourceProviderID's opaque to users of managers, the
manager assigns provider ids to all resources added.
Diffs
-----
src/resource_provider/manager.cpp 44e1576d4462295d404429d51682134306047462
Diff: https://reviews.apache.org/r/61181/diff/1/
Testing
-------
Tested as part of https://reviews.apache.org/r/61182/.
Thanks,
Benjamin Bannier
Re: Review Request 61181: Stored subscribed resources in resource
provider manager.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61181/
-----------------------------------------------------------
(Updated Aug. 1, 2017, 7:42 p.m.)
Review request for mesos, Jie Yu and Jan Schlicht.
Bugs: MESOS-7837
https://issues.apache.org/jira/browse/MESOS-7837
Repository: mesos
Description
-------
In order to be able to always communicate the aggregated total
resources available on all subscribed resource providers, a resource
provider manager needs to keep track of the resources of all
subscribed resource providers. This commit adds a field for that the
manager's internal data structures for that purpose.
To make assigned 'ResourceProviderID's opaque to users of managers, the
manager assigns provider ids to all resources added.
Diffs (updated)
-----
src/resource_provider/manager.cpp 44e1576d4462295d404429d51682134306047462
Diff: https://reviews.apache.org/r/61181/diff/3/
Changes: https://reviews.apache.org/r/61181/diff/2-3/
Testing
-------
Tested as part of https://reviews.apache.org/r/61182/.
Thanks,
Benjamin Bannier
Re: Review Request 61181: Stored subscribed resources in resource
provider manager.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61181/#review181882
-----------------------------------------------------------
Ship it!
Ship It!
- Jan Schlicht
On July 31, 2017, 5:08 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61181/
> -----------------------------------------------------------
>
> (Updated July 31, 2017, 5:08 p.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Bugs: MESOS-7837
> https://issues.apache.org/jira/browse/MESOS-7837
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In order to be able to always communicate the aggregated total
> resources available on all subscribed resource providers, a resource
> provider manager needs to keep track of the resources of all
> subscribed resource providers. This commit adds a field for that the
> manager's internal data structures for that purpose.
>
> To make assigned 'ResourceProviderID's opaque to users of managers, the
> manager assigns provider ids to all resources added.
>
>
> Diffs
> -----
>
> src/resource_provider/manager.cpp 44e1576d4462295d404429d51682134306047462
>
>
> Diff: https://reviews.apache.org/r/61181/diff/2/
>
>
> Testing
> -------
>
> Tested as part of https://reviews.apache.org/r/61182/.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 61181: Stored subscribed resources in resource
provider manager.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61181/
-----------------------------------------------------------
(Updated July 31, 2017, 5:08 p.m.)
Review request for mesos, Jie Yu and Jan Schlicht.
Changes
-------
Used the correct loop.
Bugs: MESOS-7837
https://issues.apache.org/jira/browse/MESOS-7837
Repository: mesos
Description
-------
In order to be able to always communicate the aggregated total
resources available on all subscribed resource providers, a resource
provider manager needs to keep track of the resources of all
subscribed resource providers. This commit adds a field for that the
manager's internal data structures for that purpose.
To make assigned 'ResourceProviderID's opaque to users of managers, the
manager assigns provider ids to all resources added.
Diffs (updated)
-----
src/resource_provider/manager.cpp 44e1576d4462295d404429d51682134306047462
Diff: https://reviews.apache.org/r/61181/diff/2/
Changes: https://reviews.apache.org/r/61181/diff/1-2/
Testing
-------
Tested as part of https://reviews.apache.org/r/61182/.
Thanks,
Benjamin Bannier
Re: Review Request 61181: Stored subscribed resources in resource
provider manager.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On July 28, 2017, 11:42 p.m., Jie Yu wrote:
> > src/resource_provider/manager.cpp
> > Lines 270-274 (patched)
> > <https://reviews.apache.org/r/61181/diff/1/?file=1784494#file1784494line270>
> >
> > That makes me think if it's easier to always rely on `Update` to report the total resources of a resource provider. `Subscribe` will just register with the manager with `ResourceProviderInfo`. The resource provider will send an update to the manager after subscription is successful?
> >
> > In that way, we don't have to worry about the ID injection in the manager any more (we do need to do validation!)
Yes, I believe that would make sense. I would however suggest to not tackle this right now, and instead defer this to the time where we have an update call available as this protocol requires e.g., slightly more advanced testing tools as one would need to maintain a connection (e.g., a proper `MockResourceProvider` to maintain connections). The change should be non-intrusive as we'd be in control of manager as well as resource providers at that point.
I added a `TODO` to https://reviews.apache.org/r/61180/ for now.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61181/#review181733
-----------------------------------------------------------
On July 31, 2017, 5:08 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61181/
> -----------------------------------------------------------
>
> (Updated July 31, 2017, 5:08 p.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Bugs: MESOS-7837
> https://issues.apache.org/jira/browse/MESOS-7837
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In order to be able to always communicate the aggregated total
> resources available on all subscribed resource providers, a resource
> provider manager needs to keep track of the resources of all
> subscribed resource providers. This commit adds a field for that the
> manager's internal data structures for that purpose.
>
> To make assigned 'ResourceProviderID's opaque to users of managers, the
> manager assigns provider ids to all resources added.
>
>
> Diffs
> -----
>
> src/resource_provider/manager.cpp 44e1576d4462295d404429d51682134306047462
>
>
> Diff: https://reviews.apache.org/r/61181/diff/2/
>
>
> Testing
> -------
>
> Tested as part of https://reviews.apache.org/r/61182/.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 61181: Stored subscribed resources in resource
provider manager.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61181/#review181733
-----------------------------------------------------------
src/resource_provider/manager.cpp
Lines 270-274 (patched)
<https://reviews.apache.org/r/61181/#comment257384>
That makes me think if it's easier to always rely on `Update` to report the total resources of a resource provider. `Subscribe` will just register with the manager with `ResourceProviderInfo`. The resource provider will send an update to the manager after subscription is successful?
In that way, we don't have to worry about the ID injection in the manager any more (we do need to do validation!)
src/resource_provider/manager.cpp
Lines 271 (patched)
<https://reviews.apache.org/r/61181/#comment257383>
Use `foreach` to be consistent with other files?
- Jie Yu
On July 27, 2017, 2:49 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61181/
> -----------------------------------------------------------
>
> (Updated July 27, 2017, 2:49 p.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Bugs: MESOS-7837
> https://issues.apache.org/jira/browse/MESOS-7837
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In order to be able to always communicate the aggregated total
> resources available on all subscribed resource providers, a resource
> provider manager needs to keep track of the resources of all
> subscribed resource providers. This commit adds a field for that the
> manager's internal data structures for that purpose.
>
> To make assigned 'ResourceProviderID's opaque to users of managers, the
> manager assigns provider ids to all resources added.
>
>
> Diffs
> -----
>
> src/resource_provider/manager.cpp 44e1576d4462295d404429d51682134306047462
>
>
> Diff: https://reviews.apache.org/r/61181/diff/1/
>
>
> Testing
> -------
>
> Tested as part of https://reviews.apache.org/r/61182/.
>
>
> Thanks,
>
> Benjamin Bannier
>
>