You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jiang Yan Xu <ya...@jxu.me> on 2015/06/09 02:38:17 UTC
Review Request 35239: Update the JSON model for Resources to display
their revocablility attribute.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35239/
-----------------------------------------------------------
Review request for mesos and Vinod Kone.
Bugs: MESOS-2776
https://issues.apache.org/jira/browse/MESOS-2776
Repository: mesos
Description
-------
See summary.
Diffs
-----
src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a
src/master/http.cpp f8ac30934352db859e73819e0656a70047bb0dc5
src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7
Diff: https://reviews.apache.org/r/35239/diff/
Testing
-------
Added a test.
Also tested with a master/slave pair.
e.g., The following state.json output shows a slave using a fixed estimator with `cpus:1;mem:512` revocable resources.
```json
"slaves": [
{
...
"resources": {
"cpus": 24,
"disk": 454767,
"mem": 71322,
"ports": "[31000-32000]"
},
"total_resources": {
"cpus": 24,
"cpus{REV}": 1,
"disk": 454767,
"mem": 71322,
"mem{REV}": 512,
"ports": "[31000-32000]"
},
"used_resources": {
"cpus": 0,
"disk": 0,
"mem": 0
}
}
],
```
Note that `resources` only looks at the resources from SlaveInfo while `total_resources` reads Master::Slave::totalResources.
Thanks,
Jiang Yan Xu
Re: Review Request 35239: Update the JSON model for Resources to
display their revocablility attribute.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35239/#review87121
-----------------------------------------------------------
Patch looks great!
Reviews applied: [35239]
All tests passed.
- Mesos ReviewBot
On June 9, 2015, 12:38 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35239/
> -----------------------------------------------------------
>
> (Updated June 9, 2015, 12:38 a.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-2776
> https://issues.apache.org/jira/browse/MESOS-2776
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a
> src/master/http.cpp f8ac30934352db859e73819e0656a70047bb0dc5
> src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7
>
> Diff: https://reviews.apache.org/r/35239/diff/
>
>
> Testing
> -------
>
> Added a test.
>
> Also tested with a master/slave pair.
>
> e.g., The following state.json output shows a slave using a fixed estimator with `cpus:1;mem:512` revocable resources.
> ```json
> "slaves": [
> {
> ...
> "resources": {
> "cpus": 24,
> "disk": 454767,
> "mem": 71322,
> "ports": "[31000-32000]"
> },
> "total_resources": {
> "cpus": 24,
> "cpus{REV}": 1,
> "disk": 454767,
> "mem": 71322,
> "mem{REV}": 512,
> "ports": "[31000-32000]"
> },
> "used_resources": {
> "cpus": 0,
> "disk": 0,
> "mem": 0
> }
> }
> ],
> ```
>
> Note that `resources` only looks at the resources from SlaveInfo while `total_resources` reads Master::Slave::totalResources.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 35239: Update the JSON model for Resources to
display their revocablility attribute.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On June 9, 2015, 11:36 a.m., Ben Mahler wrote:
> > Any reason not to just add a 'revocable_resources' instead of 'total_resources'? Looking to avoid the magic "REV" string proliferating, when we're not yet printing role here either.
> >
> > Ideally, when we add 'total_resources', we can do it in a way that captures everything we want (role, disk info, revocable info, etc) in a parseable way. Otherwise, might become difficult to make further changes without breaking people. For example, if we add 'total_resources' as done here, but then we want to show the role, how do we do that without breaking people's code that exactly matches "cpus" ?
It would be easy to do so if this were for the `total_resources/revocable_resources/resources` alone but the fact is that all Resources models are affected by this.
Additionally the following metrics also contain recovable resources and are affected one way or another.
- Slave: used_resources, offered_resources
- Task: resources
- Framework: offered_resources
Since revocable resources and un-revocable resources are not addable, they will be shown either as
```
"used_resources": {
"cpus": 24,
"cpus{REV}": 1,
"disk": 454767,
"mem": 71322,
"mem{REV}": 512,
"ports": "[31000-32000]"
},
```
Or as the following if we don't add the {REV} attribute,
```
"used_resources": {
"cpus": 24,
"cpus": 1,
"disk": 454767,
"mem": 71322,
"mem": 512,
"ports": "[31000-32000]"
},
```
, which is more confusing.
To make sure existing metrics stay unchanged we would need to filter out revocable resources from them explicitly.
```
object.values["offered_resources"] = model(slave.offeredResources - slave.offeredResources.revocable());
```
And then add a revocable version for each of them. I don't think that's what we want here?
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35239/#review87253
-----------------------------------------------------------
On June 8, 2015, 5:38 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35239/
> -----------------------------------------------------------
>
> (Updated June 8, 2015, 5:38 p.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-2776
> https://issues.apache.org/jira/browse/MESOS-2776
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a
> src/master/http.cpp f8ac30934352db859e73819e0656a70047bb0dc5
> src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7
>
> Diff: https://reviews.apache.org/r/35239/diff/
>
>
> Testing
> -------
>
> Added a test.
>
> Also tested with a master/slave pair.
>
> e.g., The following state.json output shows a slave using a fixed estimator with `cpus:1;mem:512` revocable resources.
> ```json
> "slaves": [
> {
> ...
> "resources": {
> "cpus": 24,
> "disk": 454767,
> "mem": 71322,
> "ports": "[31000-32000]"
> },
> "total_resources": {
> "cpus": 24,
> "cpus{REV}": 1,
> "disk": 454767,
> "mem": 71322,
> "mem{REV}": 512,
> "ports": "[31000-32000]"
> },
> "used_resources": {
> "cpus": 0,
> "disk": 0,
> "mem": 0
> }
> }
> ],
> ```
>
> Note that `resources` only looks at the resources from SlaveInfo while `total_resources` reads Master::Slave::totalResources.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 35239: Update the JSON model for Resources to
display their revocablility attribute.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On June 9, 2015, 11:36 a.m., Ben Mahler wrote:
> > Any reason not to just add a 'revocable_resources' instead of 'total_resources'? Looking to avoid the magic "REV" string proliferating, when we're not yet printing role here either.
> >
> > Ideally, when we add 'total_resources', we can do it in a way that captures everything we want (role, disk info, revocable info, etc) in a parseable way. Otherwise, might become difficult to make further changes without breaking people. For example, if we add 'total_resources' as done here, but then we want to show the role, how do we do that without breaking people's code that exactly matches "cpus" ?
>
> Jiang Yan Xu wrote:
> It would be easy to do so if this were for the `total_resources/revocable_resources/resources` alone but the fact is that all Resources models are affected by this.
>
> Additionally the following metrics also contain recovable resources and are affected one way or another.
>
> - Slave: used_resources, offered_resources
> - Task: resources
> - Framework: offered_resources
>
> Since revocable resources and un-revocable resources are not addable, they will be shown either as
> ```
> "used_resources": {
> "cpus": 24,
> "cpus{REV}": 1,
> "disk": 454767,
> "mem": 71322,
> "mem{REV}": 512,
> "ports": "[31000-32000]"
> },
> ```
> Or as the following if we don't add the {REV} attribute,
>
> ```
> "used_resources": {
> "cpus": 24,
> "cpus": 1,
> "disk": 454767,
> "mem": 71322,
> "mem": 512,
> "ports": "[31000-32000]"
> },
> ```
> , which is more confusing.
>
> To make sure existing metrics stay unchanged we would need to filter out revocable resources from them explicitly.
>
> ```
> object.values["offered_resources"] = model(slave.offeredResources - slave.offeredResources.revocable());
> ```
>
> And then add a revocable version for each of them. I don't think that's what we want here?
Another thing is that existing readings of `cpus`, `mem` and `disk` actually stay unchagned. So I think it's relatively safe. It only affects people who do `len(used_resources) == 3` which is unlikely. (Of course it's always a good idea to announce this explicitly).
It'll be involve a deprecation cycle to add the `role` attribute so I didn't add it here. However I agree that we should think about the path forward for modeling more extended resources attributes.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35239/#review87253
-----------------------------------------------------------
On June 8, 2015, 5:38 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35239/
> -----------------------------------------------------------
>
> (Updated June 8, 2015, 5:38 p.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-2776
> https://issues.apache.org/jira/browse/MESOS-2776
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a
> src/master/http.cpp f8ac30934352db859e73819e0656a70047bb0dc5
> src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7
>
> Diff: https://reviews.apache.org/r/35239/diff/
>
>
> Testing
> -------
>
> Added a test.
>
> Also tested with a master/slave pair.
>
> e.g., The following state.json output shows a slave using a fixed estimator with `cpus:1;mem:512` revocable resources.
> ```json
> "slaves": [
> {
> ...
> "resources": {
> "cpus": 24,
> "disk": 454767,
> "mem": 71322,
> "ports": "[31000-32000]"
> },
> "total_resources": {
> "cpus": 24,
> "cpus{REV}": 1,
> "disk": 454767,
> "mem": 71322,
> "mem{REV}": 512,
> "ports": "[31000-32000]"
> },
> "used_resources": {
> "cpus": 0,
> "disk": 0,
> "mem": 0
> }
> }
> ],
> ```
>
> Note that `resources` only looks at the resources from SlaveInfo while `total_resources` reads Master::Slave::totalResources.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 35239: Update the JSON model for Resources to
display their revocablility attribute.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35239/#review87253
-----------------------------------------------------------
Any reason not to just add a 'revocable_resources' instead of 'total_resources'? Looking to avoid the magic "REV" string proliferating, when we're not yet printing role here either.
Ideally, when we add 'total_resources', we can do it in a way that captures everything we want (role, disk info, revocable info, etc) in a parseable way. Otherwise, might become difficult to make further changes without breaking people. For example, if we add 'total_resources' as done here, but then we want to show the role, how do we do that without breaking people's code that exactly matches "cpus" ?
- Ben Mahler
On June 9, 2015, 12:38 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35239/
> -----------------------------------------------------------
>
> (Updated June 9, 2015, 12:38 a.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-2776
> https://issues.apache.org/jira/browse/MESOS-2776
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a
> src/master/http.cpp f8ac30934352db859e73819e0656a70047bb0dc5
> src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7
>
> Diff: https://reviews.apache.org/r/35239/diff/
>
>
> Testing
> -------
>
> Added a test.
>
> Also tested with a master/slave pair.
>
> e.g., The following state.json output shows a slave using a fixed estimator with `cpus:1;mem:512` revocable resources.
> ```json
> "slaves": [
> {
> ...
> "resources": {
> "cpus": 24,
> "disk": 454767,
> "mem": 71322,
> "ports": "[31000-32000]"
> },
> "total_resources": {
> "cpus": 24,
> "cpus{REV}": 1,
> "disk": 454767,
> "mem": 71322,
> "mem{REV}": 512,
> "ports": "[31000-32000]"
> },
> "used_resources": {
> "cpus": 0,
> "disk": 0,
> "mem": 0
> }
> }
> ],
> ```
>
> Note that `resources` only looks at the resources from SlaveInfo while `total_resources` reads Master::Slave::totalResources.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 35239: Update the JSON model for Resources to
display their revocablility attribute.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On June 9, 2015, 11:33 a.m., Vinod Kone wrote:
> > src/master/http.cpp, lines 226-227
> > <https://reviews.apache.org/r/35239/diff/1/?file=981171#file981171line226>
> >
> > why not just
> >
> > object.values["resources"] = model(slave.totalResources);
> >
> > because total resources represent the current state of the slave's resources.
> >
> > also, this is more consistent with "used_resources" and "offered_resources" which will now have revocable resources in them.
I was aiming for consistency between the metrics and state.json. In metrics, existing metrics `(cpus|mem|disk)_total` stay unchanged and only included data from `slave.info.resources()`.
But in replying to Ben's comment below I began to think it's probably better to just do this so I'll just do `object.values["resources"] = model(slave.totalResources);`.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35239/#review87252
-----------------------------------------------------------
On June 8, 2015, 5:38 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35239/
> -----------------------------------------------------------
>
> (Updated June 8, 2015, 5:38 p.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-2776
> https://issues.apache.org/jira/browse/MESOS-2776
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a
> src/master/http.cpp f8ac30934352db859e73819e0656a70047bb0dc5
> src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7
>
> Diff: https://reviews.apache.org/r/35239/diff/
>
>
> Testing
> -------
>
> Added a test.
>
> Also tested with a master/slave pair.
>
> e.g., The following state.json output shows a slave using a fixed estimator with `cpus:1;mem:512` revocable resources.
> ```json
> "slaves": [
> {
> ...
> "resources": {
> "cpus": 24,
> "disk": 454767,
> "mem": 71322,
> "ports": "[31000-32000]"
> },
> "total_resources": {
> "cpus": 24,
> "cpus{REV}": 1,
> "disk": 454767,
> "mem": 71322,
> "mem{REV}": 512,
> "ports": "[31000-32000]"
> },
> "used_resources": {
> "cpus": 0,
> "disk": 0,
> "mem": 0
> }
> }
> ],
> ```
>
> Note that `resources` only looks at the resources from SlaveInfo while `total_resources` reads Master::Slave::totalResources.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 35239: Update the JSON model for Resources to
display their revocablility attribute.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35239/#review87252
-----------------------------------------------------------
src/master/http.cpp
<https://reviews.apache.org/r/35239/#comment139566>
why not just
object.values["resources"] = model(slave.totalResources);
because total resources represent the current state of the slave's resources.
also, this is more consistent with "used_resources" and "offered_resources" which will now have revocable resources in them.
- Vinod Kone
On June 9, 2015, 12:38 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35239/
> -----------------------------------------------------------
>
> (Updated June 9, 2015, 12:38 a.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-2776
> https://issues.apache.org/jira/browse/MESOS-2776
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a
> src/master/http.cpp f8ac30934352db859e73819e0656a70047bb0dc5
> src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7
>
> Diff: https://reviews.apache.org/r/35239/diff/
>
>
> Testing
> -------
>
> Added a test.
>
> Also tested with a master/slave pair.
>
> e.g., The following state.json output shows a slave using a fixed estimator with `cpus:1;mem:512` revocable resources.
> ```json
> "slaves": [
> {
> ...
> "resources": {
> "cpus": 24,
> "disk": 454767,
> "mem": 71322,
> "ports": "[31000-32000]"
> },
> "total_resources": {
> "cpus": 24,
> "cpus{REV}": 1,
> "disk": 454767,
> "mem": 71322,
> "mem{REV}": 512,
> "ports": "[31000-32000]"
> },
> "used_resources": {
> "cpus": 0,
> "disk": 0,
> "mem": 0
> }
> }
> ],
> ```
>
> Note that `resources` only looks at the resources from SlaveInfo while `total_resources` reads Master::Slave::totalResources.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>