You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Meng Zhu <mz...@mesosphere.io> on 2019/02/27 01:28:40 UTC
Review Request 70061: Added method `fromScalarResources` in
`ResourceQuantities`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70061/
-----------------------------------------------------------
Review request for mesos and Benjamin Mahler.
Bugs: MESOS-9608
https://issues.apache.org/jira/browse/MESOS-9608
Repository: mesos
Description
-------
This method takes all the scalar type `Resource` entries in
the `Resources` and combine them to a `ResourceQuantities`.
Only the resource name and its scalar value are used and the
rest of the meta-data is ignored. Non-scalar resource entries
are silently dropped. Since internal resources are always
validated to have positive scalar values, this conversion will
always succeed.
Also added a dedicated test.
Diffs
-----
src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae
src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7
src/tests/resource_quantities_tests.cpp 435a4949b95e9a83be73781388eb4be9c7da695b
Diff: https://reviews.apache.org/r/70061/diff/1/
Testing
-------
make check
Also added a dedicated test
Thanks,
Meng Zhu
Re: Review Request 70061: Added method `fromScalarResources` in
`ResourceQuantities`.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70061/#review213568
-----------------------------------------------------------
Ship it!
This seems to make `Resources::createStrippedScalarQuantity()` unnecessary and callers can just do `ResourceQuantities.fromScalarResources(r.scalars())` which should actually be a bit more efficient since we only copy the shared pointers rather than construct new `Resource` objects.
- Benjamin Mahler
On Feb. 27, 2019, 1:28 a.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70061/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2019, 1:28 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-9608
> https://issues.apache.org/jira/browse/MESOS-9608
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This method takes all the scalar type `Resource` entries in
> the `Resources` and combine them to a `ResourceQuantities`.
> Only the resource name and its scalar value are used and the
> rest of the meta-data is ignored. Non-scalar resource entries
> are silently dropped. Since internal resources are always
> validated to have positive scalar values, this conversion will
> always succeed.
>
> Also added a dedicated test.
>
>
> Diffs
> -----
>
> src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae
> src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7
> src/tests/resource_quantities_tests.cpp 435a4949b95e9a83be73781388eb4be9c7da695b
>
>
> Diff: https://reviews.apache.org/r/70061/diff/2/
>
>
> Testing
> -------
>
> make check
> Also added a dedicated test
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 70061: Added method `fromScalarResources` in
`ResourceQuantities`.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On Feb. 27, 2019, 8:28 a.m., Benjamin Mahler wrote:
> > Can you place https://reviews.apache.org/r/70062/ as the first patch in front of this one? That would make the absence semantics clear when looking at this patch..
There are dependencies. I hid the direct mutation interfaces in r/70062/. Since some of the sorter code is already using the map interface, we need this conversion method before we can hide the map interface. Hence this is posted first.
> On Feb. 27, 2019, 8:28 a.m., Benjamin Mahler wrote:
> > src/common/resource_quantities.hpp
> > Lines 91 (patched)
> > <https://reviews.apache.org/r/70061/diff/1/?file=2127045#file2127045line91>
> >
> > This seems prone to misreading? I would expect that it mandates that all the provided resources are scalar, however it doesn't and it just drops any non-scalars. Seems unintuitive?
> >
> > Why do we need this? Why not just compose together?
> >
> > ```
> > static ResourceQuantities fromResources(const Resources& resources);
> >
> > Resource quantities = ResourceQuantities::fromResources(r.scalars());
> > ```
> >
> > Any reason not to have construction from `Resources` as a constructor?
OK, just take in scalar resources makes sense. I was thinking of performance, we will scan and do a copy (albeit not that expensive due to cow). But I think your suggestion is more intuitive.
The reason of not using constructor is naming. I want to put "scalarResources" somewhere in the name to differentiate this from future resources conversion method that also convert non-scalar resources.
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70061/#review213258
-----------------------------------------------------------
On Feb. 26, 2019, 5:28 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70061/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2019, 5:28 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-9608
> https://issues.apache.org/jira/browse/MESOS-9608
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This method takes all the scalar type `Resource` entries in
> the `Resources` and combine them to a `ResourceQuantities`.
> Only the resource name and its scalar value are used and the
> rest of the meta-data is ignored. Non-scalar resource entries
> are silently dropped. Since internal resources are always
> validated to have positive scalar values, this conversion will
> always succeed.
>
> Also added a dedicated test.
>
>
> Diffs
> -----
>
> src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae
> src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7
> src/tests/resource_quantities_tests.cpp 435a4949b95e9a83be73781388eb4be9c7da695b
>
>
> Diff: https://reviews.apache.org/r/70061/diff/2/
>
>
> Testing
> -------
>
> make check
> Also added a dedicated test
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 70061: Added method `fromScalarResources` in
`ResourceQuantities`.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70061/#review213258
-----------------------------------------------------------
Can you place https://reviews.apache.org/r/70062/ as the first patch in front of this one? That would make the absence semantics clear when looking at this patch..
src/common/resource_quantities.hpp
Lines 91 (patched)
<https://reviews.apache.org/r/70061/#comment299182>
This seems prone to misreading? I would expect that it mandates that all the provided resources are scalar, however it doesn't and it just drops any non-scalars. Seems unintuitive?
Why do we need this? Why not just compose together?
```
static ResourceQuantities fromResources(const Resources& resources);
Resource quantities = ResourceQuantities::fromResources(r.scalars());
```
Any reason not to have construction from `Resources` as a constructor?
- Benjamin Mahler
On Feb. 27, 2019, 1:28 a.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70061/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2019, 1:28 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-9608
> https://issues.apache.org/jira/browse/MESOS-9608
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This method takes all the scalar type `Resource` entries in
> the `Resources` and combine them to a `ResourceQuantities`.
> Only the resource name and its scalar value are used and the
> rest of the meta-data is ignored. Non-scalar resource entries
> are silently dropped. Since internal resources are always
> validated to have positive scalar values, this conversion will
> always succeed.
>
> Also added a dedicated test.
>
>
> Diffs
> -----
>
> src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae
> src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7
> src/tests/resource_quantities_tests.cpp 435a4949b95e9a83be73781388eb4be9c7da695b
>
>
> Diff: https://reviews.apache.org/r/70061/diff/1/
>
>
> Testing
> -------
>
> make check
> Also added a dedicated test
>
>
> Thanks,
>
> Meng Zhu
>
>