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