Re: Review Request 63523: Updated `Allocator::updateAllocation` to take `ResourceConversion`s.

2017-11-06 Thread Jie Yu 


> On Nov. 6, 2017, 3:03 p.m., Benjamin Bannier wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 5001-5003 (original), 5015-5017 (patched)
> > 
> >
> > This is weird now since `launch` is only used to calculate the 
> > expectation below.
> > 
> > We should either try to inject it in the allocator just like `create`, 
> > or try to get rid of it completely.

I get rid of it completely! thanks for catching this!


- Jie


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


On Nov. 2, 2017, 7:36 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63523/
> ---
> 
> (Updated Nov. 2, 2017, 7:36 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Gaston Kleiman, 
> Greg Mann, Jan Schlicht, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Instead of taking offer operations, it's more clear to make allocator
> not aware of offer operations. This patch changes the interface of the
> `updateAllocation` call to take `ResourceConversion`s.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> 537658bf4e68ac7f271248af0e1e35df2b19aef8 
>   src/master/allocator/mesos/allocator.hpp 
> 903edf68e812e022cddedd40a2cb51726b2a181b 
>   src/master/allocator/mesos/hierarchical.hpp 
> c2346054b2c98516f15ab8ce2dc798224ff4def4 
>   src/master/allocator/mesos/hierarchical.cpp 
> 848e9da1b61f411449c0cd2409e98ec733ac10ee 
>   src/master/master.cpp e047462018af50b9047b8939e922d80c4771fb28 
>   src/tests/allocator.hpp f1c0d148886b7b2936918b7d7c77c03ee766ee4a 
>   src/tests/hierarchical_allocator_tests.cpp 
> 48b48ad70f44cc2232c2a29699267027f9937b8a 
> 
> 
> Diff: https://reviews.apache.org/r/63523/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 63523: Updated `Allocator::updateAllocation` to take `ResourceConversion`s.

2017-11-06 Thread Jie Yu 


> On Nov. 6, 2017, 3:03 p.m., Benjamin Bannier wrote:
> > src/master/master.cpp
> > Lines 4888-4889 (original)
> > 
> >
> > This is weird. Any idea why we did inject this information into 
> > allocators? Is there any expectation that a custom allocator should be able 
> > to access this information?

It's for shared resources. `updateAllocation` expect task launch operation so 
that it can calculate how much new "shared" resources should be allocated. See 
my response above.


- Jie


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


On Nov. 2, 2017, 7:36 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63523/
> ---
> 
> (Updated Nov. 2, 2017, 7:36 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Gaston Kleiman, 
> Greg Mann, Jan Schlicht, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Instead of taking offer operations, it's more clear to make allocator
> not aware of offer operations. This patch changes the interface of the
> `updateAllocation` call to take `ResourceConversion`s.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> 537658bf4e68ac7f271248af0e1e35df2b19aef8 
>   src/master/allocator/mesos/allocator.hpp 
> 903edf68e812e022cddedd40a2cb51726b2a181b 
>   src/master/allocator/mesos/hierarchical.hpp 
> c2346054b2c98516f15ab8ce2dc798224ff4def4 
>   src/master/allocator/mesos/hierarchical.cpp 
> 848e9da1b61f411449c0cd2409e98ec733ac10ee 
>   src/master/master.cpp e047462018af50b9047b8939e922d80c4771fb28 
>   src/tests/allocator.hpp f1c0d148886b7b2936918b7d7c77c03ee766ee4a 
>   src/tests/hierarchical_allocator_tests.cpp 
> 48b48ad70f44cc2232c2a29699267027f9937b8a 
> 
> 
> Diff: https://reviews.apache.org/r/63523/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 63523: Updated `Allocator::updateAllocation` to take `ResourceConversion`s.

2017-11-06 Thread Jie Yu 


> On Nov. 6, 2017, 3:03 p.m., Benjamin Bannier wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 888-896 (original), 809-817 (patched)
> > 
> >
> > This could be more general, and I am not sure I agree with the general 
> > direction the `TODO` outlines. In the end we only need to handle 
> > conversions which actually consume resources here, and can drop all which 
> > don't. This can in principle be independent from e.g., launches on shared 
> > resources.

Currently, for shared resources, we actually "increase" the allocation during 
task launch, thus the reason for calling `updateAllocation` here. However, for 
those shared resources, the newly allocated "shared" resources are not in the 
"total" resources (which I think is a bug).

I chatted with YanX on this, I think the best way to remove this TODO is to 
update agent's total resources during allocation for shared resources so that 
any additional allocation due to task launch can be dealt with the same way as 
other resources.


- Jie


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


On Nov. 2, 2017, 7:36 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63523/
> ---
> 
> (Updated Nov. 2, 2017, 7:36 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Gaston Kleiman, 
> Greg Mann, Jan Schlicht, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Instead of taking offer operations, it's more clear to make allocator
> not aware of offer operations. This patch changes the interface of the
> `updateAllocation` call to take `ResourceConversion`s.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> 537658bf4e68ac7f271248af0e1e35df2b19aef8 
>   src/master/allocator/mesos/allocator.hpp 
> 903edf68e812e022cddedd40a2cb51726b2a181b 
>   src/master/allocator/mesos/hierarchical.hpp 
> c2346054b2c98516f15ab8ce2dc798224ff4def4 
>   src/master/allocator/mesos/hierarchical.cpp 
> 848e9da1b61f411449c0cd2409e98ec733ac10ee 
>   src/master/master.cpp e047462018af50b9047b8939e922d80c4771fb28 
>   src/tests/allocator.hpp f1c0d148886b7b2936918b7d7c77c03ee766ee4a 
>   src/tests/hierarchical_allocator_tests.cpp 
> 48b48ad70f44cc2232c2a29699267027f9937b8a 
> 
> 
> Diff: https://reviews.apache.org/r/63523/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 63523: Updated `Allocator::updateAllocation` to take `ResourceConversion`s.

2017-11-06 Thread Benjamin Bannier 

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


Fix it, then Ship it!





src/master/allocator/mesos/hierarchical.cpp
Line 828 (original), 768 (patched)


This can be a `const` ref.



src/master/allocator/mesos/hierarchical.cpp
Lines 888-896 (original), 809-817 (patched)


This could be more general, and I am not sure I agree with the general 
direction the `TODO` outlines. In the end we only need to handle conversions 
which actually consume resources here, and can drop all which don't. This can 
in principle be independent from e.g., launches on shared resources.



src/master/master.cpp
Lines 4890 (patched)


Not yours, but this could be a `const` ref.



src/master/master.cpp
Lines 4888-4889 (original)


This is weird. Any idea why we did inject this information into allocators? 
Is there any expectation that a custom allocator should be able to access this 
information?



src/tests/hierarchical_allocator_tests.cpp
Lines 5001-5003 (original), 5015-5017 (patched)


This is weird now since `launch` is only used to calculate the expectation 
below.

We should either try to inject it in the allocator just like `create`, or 
try to get rid of it completely.


- Benjamin Bannier


On Nov. 2, 2017, 8:36 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63523/
> ---
> 
> (Updated Nov. 2, 2017, 8:36 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Gaston Kleiman, 
> Greg Mann, Jan Schlicht, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Instead of taking offer operations, it's more clear to make allocator
> not aware of offer operations. This patch changes the interface of the
> `updateAllocation` call to take `ResourceConversion`s.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> 537658bf4e68ac7f271248af0e1e35df2b19aef8 
>   src/master/allocator/mesos/allocator.hpp 
> 903edf68e812e022cddedd40a2cb51726b2a181b 
>   src/master/allocator/mesos/hierarchical.hpp 
> c2346054b2c98516f15ab8ce2dc798224ff4def4 
>   src/master/allocator/mesos/hierarchical.cpp 
> 848e9da1b61f411449c0cd2409e98ec733ac10ee 
>   src/master/master.cpp e047462018af50b9047b8939e922d80c4771fb28 
>   src/tests/allocator.hpp f1c0d148886b7b2936918b7d7c77c03ee766ee4a 
>   src/tests/hierarchical_allocator_tests.cpp 
> 48b48ad70f44cc2232c2a29699267027f9937b8a 
> 
> 
> Diff: https://reviews.apache.org/r/63523/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 63523: Updated `Allocator::updateAllocation` to take `ResourceConversion`s.

2017-11-02 Thread Mesos Reviewbot 

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



Bad patch!

Reviews applied: [63523, 63511, 63485, 63484, 63483, 63482, 63481, 63432, 
63480, 63414, 63413, 63213, 63212, 63211, 63210, 63200, 63017, 59989, 59988, 
59987, 60109]

Failed command: python support/apply-reviews.py -n -r 59988

Error:
2017-11-03 01:37:31 URL:https://reviews.apache.org/r/59988/diff/raw/ 
[1148/1148] -> "59988.patch" [1]
error: patch failed: 3rdparty/stout/tests/protobuf_tests.proto:24
error: 3rdparty/stout/tests/protobuf_tests.proto: patch does not apply

Full log: https://builds.apache.org/job/Mesos-Reviewbot/19976/console

- Mesos Reviewbot


On Nov. 2, 2017, 7:36 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63523/
> ---
> 
> (Updated Nov. 2, 2017, 7:36 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Gaston Kleiman, 
> Greg Mann, Jan Schlicht, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Instead of taking offer operations, it's more clear to make allocator
> not aware of offer operations. This patch changes the interface of the
> `updateAllocation` call to take `ResourceConversion`s.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> 537658bf4e68ac7f271248af0e1e35df2b19aef8 
>   src/master/allocator/mesos/allocator.hpp 
> 903edf68e812e022cddedd40a2cb51726b2a181b 
>   src/master/allocator/mesos/hierarchical.hpp 
> c2346054b2c98516f15ab8ce2dc798224ff4def4 
>   src/master/allocator/mesos/hierarchical.cpp 
> 848e9da1b61f411449c0cd2409e98ec733ac10ee 
>   src/master/master.cpp e047462018af50b9047b8939e922d80c4771fb28 
>   src/tests/allocator.hpp f1c0d148886b7b2936918b7d7c77c03ee766ee4a 
>   src/tests/hierarchical_allocator_tests.cpp 
> 48b48ad70f44cc2232c2a29699267027f9937b8a 
> 
> 
> Diff: https://reviews.apache.org/r/63523/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 63523: Updated `Allocator::updateAllocation` to take `ResourceConversion`s.

2017-11-02 Thread Mesos Reviewbot Windows 

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



FAIL: Failed to apply the dependent review: 59988.

Failed command: `python.exe .\support\apply-reviews.py -n -r 59988`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/63523

Relevant logs:

- 
[apply-review-59988-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/63523/logs/apply-review-59988-stdout.log):

```
error: patch failed: 3rdparty/stout/tests/protobuf_tests.proto:24
error: 3rdparty/stout/tests/protobuf_tests.proto: patch does not apply
```

- Mesos Reviewbot Windows


On Nov. 2, 2017, 7:36 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63523/
> ---
> 
> (Updated Nov. 2, 2017, 7:36 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Gaston Kleiman, 
> Greg Mann, Jan Schlicht, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Instead of taking offer operations, it's more clear to make allocator
> not aware of offer operations. This patch changes the interface of the
> `updateAllocation` call to take `ResourceConversion`s.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> 537658bf4e68ac7f271248af0e1e35df2b19aef8 
>   src/master/allocator/mesos/allocator.hpp 
> 903edf68e812e022cddedd40a2cb51726b2a181b 
>   src/master/allocator/mesos/hierarchical.hpp 
> c2346054b2c98516f15ab8ce2dc798224ff4def4 
>   src/master/allocator/mesos/hierarchical.cpp 
> 848e9da1b61f411449c0cd2409e98ec733ac10ee 
>   src/master/master.cpp e047462018af50b9047b8939e922d80c4771fb28 
>   src/tests/allocator.hpp f1c0d148886b7b2936918b7d7c77c03ee766ee4a 
>   src/tests/hierarchical_allocator_tests.cpp 
> 48b48ad70f44cc2232c2a29699267027f9937b8a 
> 
> 
> Diff: https://reviews.apache.org/r/63523/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Review Request 63523: Updated `Allocator::updateAllocation` to take `ResourceConversion`s.

2017-11-02 Thread Jie Yu 

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

Review request for mesos, Benjamin Bannier, Benjamin Mahler, Gaston Kleiman, 
Greg Mann, Jan Schlicht, Vinod Kone, and Jiang Yan Xu.


Repository: mesos


Description
---

Instead of taking offer operations, it's more clear to make allocator
not aware of offer operations. This patch changes the interface of the
`updateAllocation` call to take `ResourceConversion`s.


Diffs
-

  include/mesos/allocator/allocator.hpp 
537658bf4e68ac7f271248af0e1e35df2b19aef8 
  src/master/allocator/mesos/allocator.hpp 
903edf68e812e022cddedd40a2cb51726b2a181b 
  src/master/allocator/mesos/hierarchical.hpp 
c2346054b2c98516f15ab8ce2dc798224ff4def4 
  src/master/allocator/mesos/hierarchical.cpp 
848e9da1b61f411449c0cd2409e98ec733ac10ee 
  src/master/master.cpp e047462018af50b9047b8939e922d80c4771fb28 
  src/tests/allocator.hpp f1c0d148886b7b2936918b7d7c77c03ee766ee4a 
  src/tests/hierarchical_allocator_tests.cpp 
48b48ad70f44cc2232c2a29699267027f9937b8a 


Diff: https://reviews.apache.org/r/63523/diff/1/


Testing
---

make check


Thanks,

Jie Yu