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 70062: Refactored and augmented `class ResourceQuantities`.
Date Thu, 28 Feb 2019 01:19:28 GMT


> On Feb. 27, 2019, 9:04 a.m., Benjamin Mahler wrote:
> > src/common/resource_quantities.cpp
> > Lines 61-64 (original), 61-64 (patched)
> > <https://reviews.apache.org/r/70062/diff/1/?file=2127049#file2127049line61>
> >
> >     Ditto comment below, it seems 0 should be allowed here.

Agreed. Done.


> On Feb. 27, 2019, 9:04 a.m., Benjamin Mahler wrote:
> > src/common/resource_quantities.cpp
> > Lines 141-178 (patched)
> > <https://reviews.apache.org/r/70062/diff/1/?file=2127049#file2127049line141>
> >
> >     Can you make these more symmetrical?
> >     
> >     If we want to keep the [] operator, why don't we use it in -= and also use a
new .erase function?
> >     
> >     ```
> >       // TODO: Walk both vectors at the same time, rather than re-searching.
> >       foreach (auto& that, those) {
> >         (*this)[that.first] -= that.second;
> >         
> >         if ((*this)[that.first] == 0) {
> >           erase(that.first);
> >         }
> >       }
> >     
> >       return *this;
> >     ```
> >     
> >     There doesn't seem to be a big benefit from the existing code in this patch
for the complexity it has? It still is re-searching the list from the beginning each iteration
as opposed to walking both lists assuming alphabetical ordering for better performance.

Made them symmetrical, both take advantage of the ordering and are now one-pass. If we just
do the loop thing, it kinds of defeat the purpose of ordering. Anyway, these are going to
be used in performance critical places, so a bit of performance gain for the complexity seems
to be worth it.

`[]` is still kept, mainly for the (current and future) constructors and conversion methods.


> On Feb. 27, 2019, 9:04 a.m., Benjamin Mahler wrote:
> > src/master/allocator/sorter/drf/sorter.hpp
> > Line 347 (original), 347 (patched)
> > <https://reviews.apache.org/r/70062/diff/1/?file=2127050#file2127050line347>
> >
> >     Seems like we sould just allow an implicit constructor to make this seamless?
> >     
> >     ```
> >           totals += quantitiesToAdd;
> >     ```

I want to distinguish this from future conversations interfaces where non-scalar resources
are also converted into quantities.


> On Feb. 27, 2019, 9:04 a.m., Benjamin Mahler wrote:
> > src/tests/resource_quantities_tests.cpp
> > Lines 116-118 (patched)
> > <https://reviews.apache.org/r/70062/diff/1/?file=2127054#file2127054line123>
> >
> >     Is this consistent with Resources? This seems fine and it should just be not
stored.

Yep, fixed.


- Meng


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70062/#review213260
-----------------------------------------------------------


On Feb. 26, 2019, 5:29 p.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70062/
> -----------------------------------------------------------
> 
> (Updated Feb. 26, 2019, 5:29 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9608
>     https://issues.apache.org/jira/browse/MESOS-9608
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch removed the map interface of
> `class ResourceQuantities`, added a few built-in
> arithmetic operations. Now, absent resource items imply
> there is no (zero) such resources.
> 
> Also added a to-do to add `class ResourceLimits` which
> is similar but treats absent resource entries as having
> infinite amount of such resource.
> 
> Also changed affected call sites and tests accordingly.
> 
> 
> Diffs
> -----
> 
>   src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae 
>   src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7 
>   src/master/allocator/sorter/drf/sorter.hpp e64c9ad3520a601f7854e807ef5306d5bffc0ff8

>   src/master/allocator/sorter/drf/sorter.cpp b128df08e3c93d3d1a75c637cbed359c2cb8cda4

>   src/master/allocator/sorter/random/sorter.hpp 4f230ec740e2f80d5333c61c5b23d9a631bdb273

>   src/master/allocator/sorter/random/sorter.cpp f578ef19b4dee9cf9c7c99a8988829ecde70ed6d

>   src/tests/resource_quantities_tests.cpp 435a4949b95e9a83be73781388eb4be9c7da695b 
> 
> 
> Diff: https://reviews.apache.org/r/70062/diff/2/
> 
> 
> Testing
> -------
> 
> make check
> Dedicated tests are added in the subsequent patch.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>


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