Re: 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/#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
Re: 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/#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.
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.
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.
---
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
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
