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/28 02:09:29 UTC
Re: Review Request 70320: Used `ResourceQuantities` in `__allocate()`
when appropriate.
> On March 27, 2019, 7:41 a.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1928 (patched)
> > <https://reviews.apache.org/r/70320/diff/1/?file=2134448#file2134448line1938>
> >
> > Rather than adding this additional function, can't `shrinkResoures` take a `ResourceQuantities`?
We need to create a map-like a copy somewhere. `shrinkResoures` needs a copy of its own to do bookkeeping:
https://github.com/apache/mesos/blob/c10def96653fb99e309459476f5bc09c1da686ee/src/master/allocator/mesos/hierarchical.cpp#L1666
Or do you mean to create the map in the `shrinkResoures` function? My concern with that is currently `shrinkResources` has this semantic:
>If a resource does not have a target quantity provided, it will not be shrunk.
This is not compatible with `ResourceQuantities`. Given the semantic of `ResourceQuantities`, one would expect `shrinkResources` should drop (shrink to zero) for resources that are absent in the input `ResourceQuantities`.
Of course, since we are the only consumer of the function here, we could change the function semantic. But I do not see much benefits since, as mentioned above, we need to pass a map anyway.
Another thing to consider is we might need to get a copy of the scalar map somewhere else where direct mutations are needed.
> On March 27, 2019, 7:41 a.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 2088-2091 (original), 2079-2082 (patched)
> > <https://reviews.apache.org/r/70320/diff/1/?file=2134448#file2134448line2106>
> >
> > This looks like a `ResourceQuantities`?
Yes and no. Below:
```
if (!sufficientHeadroom) {
toAllocate -= headroomToAllocate;
}
```
We need that as `Resources`.
But I do notice that we convert it to `ResourceQuantities` twice below. Might as well keep one in the scope.
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70320/#review214106
-----------------------------------------------------------
On March 26, 2019, 10:31 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70320/
> -----------------------------------------------------------
>
> (Updated March 26, 2019, 10:31 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-9504
> https://issues.apache.org/jira/browse/MESOS-9504
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Replaced `Resources` with `ResourceQuantities` when
> appropriate in `__allocate()`. This simplifies the
> code and improves performance.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 36bf90baf413e99c1580d516dfac0f074335d322
> src/master/allocator/mesos/hierarchical.cpp 8bc749903b8ceb09a02e260919377483479302b5
>
>
> Diff: https://reviews.apache.org/r/70320/diff/2/
>
>
> Testing
> -------
>
> make check
> Benchmark results in r/70330
>
>
> Thanks,
>
> Meng Zhu
>
>