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/03/07 07:56:12 UTC
Review Request 70151: Added `class ResourceLimits`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70151/
-----------------------------------------------------------
Review request for mesos and Benjamin Mahler.
Bugs: MESOS-9608
https://issues.apache.org/jira/browse/MESOS-9608
Repository: mesos
Description
-------
Similar to `ResourceQuantities`. `ResourceLimits`
is an efficient collection of resource quantities.
The main difference lies in the absence semantic.
Absent entry in `ResourceQuantities` implies zero
quantity while in `ResourceLimits`, it implies no
limit (i.e. infinite amount). Also, due to the
absence semantic, zero values in `ResourceLimits`
are not dropped.
Diffs
-----
src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae
src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7
Diff: https://reviews.apache.org/r/70151/diff/1/
Testing
-------
test added in r/70152
Thanks,
Meng Zhu
Re: Review Request 70151: Added `class ResourceLimits`.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70151/#review213748
-----------------------------------------------------------
Fix it, then Ship it!
src/common/resource_quantities.cpp
Lines 377 (patched)
<https://reviews.apache.org/r/70151/#comment299738>
Newline before comment?
- Benjamin Mahler
On March 7, 2019, 7:56 a.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70151/
> -----------------------------------------------------------
>
> (Updated March 7, 2019, 7:56 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-9608
> https://issues.apache.org/jira/browse/MESOS-9608
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Similar to `ResourceQuantities`. `ResourceLimits`
> is an efficient collection of resource quantities.
> The main difference lies in the absence semantic.
> Absent entry in `ResourceQuantities` implies zero
> quantity while in `ResourceLimits`, it implies no
> limit (i.e. infinite amount). Also, due to the
> absence semantic, zero values in `ResourceLimits`
> are not dropped.
>
>
> Diffs
> -----
>
> src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae
> src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7
>
>
> Diff: https://reviews.apache.org/r/70151/diff/3/
>
>
> Testing
> -------
>
> test added in r/70152
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 70151: Added `class ResourceLimits`.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On March 11, 2019, 3:49 p.m., Benjamin Mahler wrote:
> > Looks good, just the same comments as the earlier review for ResourceQuantities
Thanks. The patch has been updated.
> On March 11, 2019, 3:49 p.m., Benjamin Mahler wrote:
> > src/common/resource_quantities.cpp
> > Lines 334 (patched)
> > <https://reviews.apache.org/r/70151/diff/1/?file=2129475#file2129475line334>
> >
> > Ideally auto&& or const auto&, but as with the other review, probably can change 'get' to return non-Option Value::Scalar or remove it until needed?
I kept the method for symmetry with `ResourceQuantities`. I think it will be needed very soon in the allocator. Also, I kept the `Option` interface and noted that `None()` implies no-limit i.e. infinite.
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70151/#review213609
-----------------------------------------------------------
On March 6, 2019, 11:56 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70151/
> -----------------------------------------------------------
>
> (Updated March 6, 2019, 11:56 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-9608
> https://issues.apache.org/jira/browse/MESOS-9608
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Similar to `ResourceQuantities`. `ResourceLimits`
> is an efficient collection of resource quantities.
> The main difference lies in the absence semantic.
> Absent entry in `ResourceQuantities` implies zero
> quantity while in `ResourceLimits`, it implies no
> limit (i.e. infinite amount). Also, due to the
> absence semantic, zero values in `ResourceLimits`
> are not dropped.
>
>
> Diffs
> -----
>
> src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae
> src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7
>
>
> Diff: https://reviews.apache.org/r/70151/diff/1/
>
>
> Testing
> -------
>
> test added in r/70152
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 70151: Added `class ResourceLimits`.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70151/#review213609
-----------------------------------------------------------
Looks good, just the same comments as the earlier review for ResourceQuantities
src/common/resource_quantities.cpp
Lines 334 (patched)
<https://reviews.apache.org/r/70151/#comment299595>
Ideally auto&& or const auto&, but as with the other review, probably can change 'get' to return non-Option Value::Scalar or remove it until needed?
- Benjamin Mahler
On March 7, 2019, 7:56 a.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70151/
> -----------------------------------------------------------
>
> (Updated March 7, 2019, 7:56 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-9608
> https://issues.apache.org/jira/browse/MESOS-9608
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Similar to `ResourceQuantities`. `ResourceLimits`
> is an efficient collection of resource quantities.
> The main difference lies in the absence semantic.
> Absent entry in `ResourceQuantities` implies zero
> quantity while in `ResourceLimits`, it implies no
> limit (i.e. infinite amount). Also, due to the
> absence semantic, zero values in `ResourceLimits`
> are not dropped.
>
>
> Diffs
> -----
>
> src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae
> src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7
>
>
> Diff: https://reviews.apache.org/r/70151/diff/1/
>
>
> Testing
> -------
>
> test added in r/70152
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 70151: Added `class ResourceLimits`.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On March 13, 2019, 12:12 p.m., Benjamin Mahler wrote:
> > src/common/resource_quantities.cpp
> > Lines 398 (patched)
> > <https://reviews.apache.org/r/70151/diff/2/?file=2131663#file2131663line398>
> >
> > Ah, this is indeed a little more concrete than saying "items" like above, but maybe we can use "limits"?
> >
> > ```
> > Left has finite limits for resources that right has no limits for.
> > ```
Sounds good. Thanks.
> On March 13, 2019, 12:12 p.m., Benjamin Mahler wrote:
> > src/common/resource_quantities.cpp
> > Lines 416 (patched)
> > <https://reviews.apache.org/r/70151/diff/2/?file=2131663#file2131663line416>
> >
> > Hm.. don't you need to return here?
> >
> > Looks like it's going to overwrite then insert an extra entry below after the loop? i.e. seems like a bug?
Oops, guilty. I think currently this is not possible to hit this because duplicate names are not allowed in the input string and protobuf map. That is why the tests did not catch this. But in the future, we may hit this when, e.g., constructing limits from resources with meta-data.
Thanks for catching this!
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70151/#review213676
-----------------------------------------------------------
On March 6, 2019, 11:56 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70151/
> -----------------------------------------------------------
>
> (Updated March 6, 2019, 11:56 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-9608
> https://issues.apache.org/jira/browse/MESOS-9608
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Similar to `ResourceQuantities`. `ResourceLimits`
> is an efficient collection of resource quantities.
> The main difference lies in the absence semantic.
> Absent entry in `ResourceQuantities` implies zero
> quantity while in `ResourceLimits`, it implies no
> limit (i.e. infinite amount). Also, due to the
> absence semantic, zero values in `ResourceLimits`
> are not dropped.
>
>
> Diffs
> -----
>
> src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae
> src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7
>
>
> Diff: https://reviews.apache.org/r/70151/diff/3/
>
>
> Testing
> -------
>
> test added in r/70152
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 70151: Added `class ResourceLimits`.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70151/#review213676
-----------------------------------------------------------
Looks like a bug was introduced? See below
src/common/resource_quantities.hpp
Lines 192 (patched)
<https://reviews.apache.org/r/70151/#comment299672>
Ditto from previous review, `get` seems more readable?
```
limits.get("cpus")
vs
limits.limit("cpus")
```
The first seems to suggest I'm getting the cpus limit, the second seems to suggest I'm limiting cpus (i.e. the action looks to be "limit cpus")
src/common/resource_quantities.cpp
Lines 374-375 (patched)
<https://reviews.apache.org/r/70151/#comment299673>
Ditto here:
```
size_t leftIndex = 0;
size_t rightIndex = 0;
while (leftIndex < size() && rightIndex < right.size()) {
...
```
src/common/resource_quantities.cpp
Lines 376-377 (patched)
<https://reviews.apache.org/r/70151/#comment299674>
Ditto here:
```
const pair<string, Value::Scalar>& left_ = limits.at(leftIndex);
const pair<string, Value::Scalar>& right_ = right.limits.at(rightIndex);
```
src/common/resource_quantities.cpp
Lines 379-380 (patched)
<https://reviews.apache.org/r/70151/#comment299675>
Ditto here, this seems to belong above the loop?
src/common/resource_quantities.cpp
Lines 382 (patched)
<https://reviews.apache.org/r/70151/#comment299676>
We probably want to be a little more concrete here to make this readable:
```
// Left has a finite limit but right has no limit.
```
Ditto below for the other cases.
src/common/resource_quantities.cpp
Lines 398 (patched)
<https://reviews.apache.org/r/70151/#comment299677>
Ah, this is indeed a little more concrete than saying "items" like above, but maybe we can use "limits"?
```
Left has finite limits for resources that right has no limits for.
```
src/common/resource_quantities.cpp
Lines 416 (patched)
<https://reviews.apache.org/r/70151/#comment299671>
Hm.. don't you need to return here?
Looks like it's going to overwrite then insert an extra entry below after the loop? i.e. seems like a bug?
- Benjamin Mahler
On March 7, 2019, 7:56 a.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70151/
> -----------------------------------------------------------
>
> (Updated March 7, 2019, 7:56 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-9608
> https://issues.apache.org/jira/browse/MESOS-9608
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Similar to `ResourceQuantities`. `ResourceLimits`
> is an efficient collection of resource quantities.
> The main difference lies in the absence semantic.
> Absent entry in `ResourceQuantities` implies zero
> quantity while in `ResourceLimits`, it implies no
> limit (i.e. infinite amount). Also, due to the
> absence semantic, zero values in `ResourceLimits`
> are not dropped.
>
>
> Diffs
> -----
>
> src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae
> src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7
>
>
> Diff: https://reviews.apache.org/r/70151/diff/2/
>
>
> Testing
> -------
>
> test added in r/70152
>
>
> Thanks,
>
> Meng Zhu
>
>