mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Meng Zhu <m...@mesosphere.io>
Subject Re: Review Request 70320: Used `ResourceQuantities` in `__allocate()` when appropriate.
Date Thu, 28 Mar 2019 02:09:29 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message