Re: Review Request 66044: Refactored resources chopping logic in allocator.

[Benjamin Mahler]

> On April 10, 2018, 10 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1607-1610 (patched)
> > 
> >
> > I assume this continues to work as the value scalar goes negative? We 
> > should probably pull this up into the header and unit test it?
> 
> Meng Zhu wrote:
> By "continues to work", I guess you expect the function to ignore 
> negative shrink? Then no, current `Resources::shrink()` will return a 
> `Resource` object with negative values with the same meta-data (if 
> shrinkable).
> 
> 
> https://github.com/apache/mesos/blob/88f5629e510d71a32bd7e0ff7ee09e150f944e72/src/v1/resources.cpp#L1296-L1315
> 
> I can fix that in a separate patch (if negative return directly).
> 
> Though I think in most use cases, the scalar values will most likely come 
> from another resource(s) object, I do not think a negative resource object 
> can be created easily (unless intensionally).

Oh I see now that the loop won't produce negative scalars, nevermind :)


- Benjamin


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


On April 10, 2018, 10:54 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66044/
> ---
> 
> (Updated April 10, 2018, 10:54 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kapil Arya, Joseph Wu, Michael 
> Park, and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a `shrinkResources()` lambda in allocator
> so that the same resource chopping logic can be re-used
> in the future, in particular, when introducing the quota
> limit.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 32e88952101d8dbbae9728478b1f5663bf46c3bb 
> 
> 
> Diff: https://reviews.apache.org/r/66044/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 66044: Refactored resources chopping logic in allocator.

[Meng Zhu]

> On April 10, 2018, 3 p.m., Benjamin Mahler wrote:
> > Looks good! Just thinking we probably should pull the shrink function up 
> > and unit test it? Feel free to do that in a second patch.

Got it. Will put it in the resource headers in a followup patch.


> On April 10, 2018, 3 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1600 (patched)
> > 
> >
> > To enable move performance improvements later, we can do:
> > 
> > ```
> > result += std::move(resource);
> > ```

Done.


> On April 10, 2018, 3 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1607-1610 (patched)
> > 
> >
> > I assume this continues to work as the value scalar goes negative? We 
> > should probably pull this up into the header and unit test it?

By "continues to work", I guess you expect the function to ignore negative 
shrink? Then no, current `Resources::shrink()` will return a `Resource` object 
with negative values with the same meta-data (if shrinkable).

https://github.com/apache/mesos/blob/88f5629e510d71a32bd7e0ff7ee09e150f944e72/src/v1/resources.cpp#L1296-L1315

I can fix that in a separate patch (if negative return directly).

Though I think in most use cases, the scalar values will most likely come from 
another resource(s) object, I do not think a negative resource object can be 
created easily (unless intensionally).


- Meng


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


On April 10, 2018, 3:54 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66044/
> ---
> 
> (Updated April 10, 2018, 3:54 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kapil Arya, Joseph Wu, Michael 
> Park, and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a `shrinkResources()` lambda in allocator
> so that the same resource chopping logic can be re-used
> in the future, in particular, when introducing the quota
> limit.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 32e88952101d8dbbae9728478b1f5663bf46c3bb 
> 
> 
> Diff: https://reviews.apache.org/r/66044/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 66044: Refactored resources chopping logic in allocator.

[Meng Zhu]

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

(Updated April 10, 2018, 3:54 p.m.)


Review request for mesos, Benjamin Mahler, Kapil Arya, Joseph Wu, Michael Park, 
and Till Toenshoff.


Repository: mesos


Description
---

Introduced a `shrinkResources()` lambda in allocator
so that the same resource chopping logic can be re-used
in the future, in particular, when introducing the quota
limit.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
32e88952101d8dbbae9728478b1f5663bf46c3bb 


Diff: https://reviews.apache.org/r/66044/diff/3/

Changes: https://reviews.apache.org/r/66044/diff/2-3/


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 66044: Refactored resources chopping logic in allocator.

[Benjamin Mahler]

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


Fix it, then Ship it!




Looks good! Just thinking we probably should pull the shrink function up and 
unit test it? Feel free to do that in a second patch.


src/master/allocator/mesos/hierarchical.cpp
Lines 1600 (patched)


To enable move performance improvements later, we can do:

```
result += std::move(resource);
```



src/master/allocator/mesos/hierarchical.cpp
Lines 1607-1610 (patched)


I assume this continues to work as the value scalar goes negative? We 
should probably pull this up into the header and unit test it?



src/master/allocator/mesos/hierarchical.cpp
Lines 1608-1609 (patched)


Ditto here for moves:

```
targetScalarQuantites[resource.name()] -= limitScalar.get();
result += std::move(resource);
```



src/master/allocator/mesos/hierarchical.cpp
Line 1868 (original), 1883 (patched)


s/(/ (/



src/master/allocator/mesos/hierarchical.cpp
Line 1870 (original), 1885 (patched)


Can you use CHECK_NOTNONE here instead of .get()?



src/master/allocator/mesos/hierarchical.cpp
Lines 1875-1876 (original), 1888-1889 (patched)


I think it's clear from the code without this comment, the comment actually 
threw me off because I was trying to figure out what "this time" was referring 
to (e.g. stage 1?).



src/master/allocator/mesos/hierarchical.cpp
Lines 1927 (patched)


Ditto here s/(/ (/



src/master/allocator/mesos/hierarchical.cpp
Lines 1929 (patched)


Ditto here CHECK_NOTNONE


- Benjamin Mahler


On March 19, 2018, 11:48 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66044/
> ---
> 
> (Updated March 19, 2018, 11:48 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kapil Arya, Joseph Wu, Michael 
> Park, and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a `shrinkResources()` lambda in allocator
> so that the same resource chopping logic can be re-used
> in the future, in particular, when introducing the quota
> limit.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 32e88952101d8dbbae9728478b1f5663bf46c3bb 
> 
> 
> Diff: https://reviews.apache.org/r/66044/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 66044: Refactored resources chopping logic in allocator.

[Meng Zhu]

> On March 16, 2018, 7:26 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1577-1590 (patched)
> > 
> >
> > This function seems rather specific to the usage in this code, which 
> > makes it not very easy to intuit its behavior. How about:
> > 
> > ```
> > // Returns the result of shrinking the provided resources down to the
> > // target scalar quantities. If a resource does not have a target
> > // quantity provided, it will not be shrunk.
> > //
> > // Note that some resources are indivisible (e.g. MOUNT volume) and
> > // may be excluded in entirety in order to acheive the target size.
> > //
> > // Note also that there may be more than 1 result that satisfies
> > // the target sizes (e.g. need to exclude 1 of 2 disks); this function
> > // will make a random choice in these cases.
> > auto shrinkResources = [](
> > const Resources& resources,
> > hashmap targetQuantities) {
> >   ...
> > };
> > ```
> > 
> > Then the caller can omit resources if it wants to acheive a default 
> > target size of 0 (i.e. `filterNonExistScalar`):
> > 
> > ```
> > Resources allocation = shrinkResources(allocation, targetQuantities);
> > 
> > allocation = allocation.filter([](const Resource& r) {
> >   return targetQuantites.contains(r.name());
> > });
> > ```

Great point! Thanks!


- Meng


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


On March 19, 2018, 4:48 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66044/
> ---
> 
> (Updated March 19, 2018, 4:48 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kapil Arya, Joseph Wu, Michael 
> Park, and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a `shrinkResources()` lambda in allocator
> so that the same resource chopping logic can be re-used
> in the future, in particular, when introducing the quota
> limit.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 32e88952101d8dbbae9728478b1f5663bf46c3bb 
> 
> 
> Diff: https://reviews.apache.org/r/66044/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 66044: Refactored resources chopping logic in allocator.

[Meng Zhu]

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

(Updated March 19, 2018, 4:48 p.m.)


Review request for mesos, Benjamin Mahler, Kapil Arya, Joseph Wu, Michael Park, 
and Till Toenshoff.


Repository: mesos


Description
---

Introduced a `shrinkResources()` lambda in allocator
so that the same resource chopping logic can be re-used
in the future, in particular, when introducing the quota
limit.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
32e88952101d8dbbae9728478b1f5663bf46c3bb 


Diff: https://reviews.apache.org/r/66044/diff/2/

Changes: https://reviews.apache.org/r/66044/diff/1-2/


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 66044: Refactored resources chopping logic in allocator.

[Benjamin Mahler]

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




src/master/allocator/mesos/hierarchical.cpp
Lines 1577-1590 (patched)


This function seems rather specific to the usage in this code, which makes 
it not very easy to intuit its behavior. How about:

```
// Returns the result of shrinking the provided resources down to the
// target scalar quantities. If a resource does not have a target
// quantity provided, it will not be shrunk.
//
// Note that some resources are indivisible (e.g. MOUNT volume) and
// may be excluded in entirety in order to acheive the target size.
//
// Note also that there may be more than 1 result that satisfies
// the target sizes (e.g. need to exclude 1 of 2 disks); this function
// will make a random choice in these cases.
auto shrinkResources = [](
const Resources& resources,
hashmap targetQuantities) {
  ...
};
```

Then the caller can omit resources if it wants to acheive a default target 
size of 0 (i.e. `filterNonExistScalar`):

```
Resources allocation = shrinkResources(allocation, targetQuantities);

allocation = allocation.filter([](const Resource& r) {
  return targetQuantites.contains(r.name());
});
```



src/master/allocator/mesos/hierarchical.cpp
Lines 1595-1603 (patched)


This comment seems to belong in the interface now that it's a function, 
otherwise the user reading the documentation for shrinkResources does not know 
that the result can vary given the input.


- Benjamin Mahler


On March 13, 2018, 10:04 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66044/
> ---
> 
> (Updated March 13, 2018, 10:04 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kapil Arya, Joseph Wu, Michael 
> Park, and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a `shrinkResources()` lambda in allocator
> so that the same resource chopping logic can be re-used
> in the future, in particular, when introducing the quota
> limit.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 0e8c2c4a52969448f99bd5f42252a84cc52b9271 
> 
> 
> Diff: https://reviews.apache.org/r/66044/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>