Re: Review Request 42332: Trigger shutdown on task pruning failure.

2016-01-14 Thread Aurora ReviewBot 

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

Ship it!


Master (4dff5da) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Jan. 15, 2016, 12:53 a.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42332/
> ---
> 
> (Updated Jan. 15, 2016, 12:53 a.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1582
> https://issues.apache.org/jira/browse/AURORA-1582
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Task pruning is critical to the operation of a large cluster. Failure to 
> prune tasks can lead to storage growing in unexpected ways leading to 
> scheduling slowdown. This patch adds shutdown on task pruning failure to 
> prevent the scheduler from entering an undefined state.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 
> 2064089937f5178b1413d386a312f4173a0e35fb 
>   
> src/test/java/org/apache/aurora/scheduler/pruning/TaskHistoryPrunerTest.java 
> 295960f13693c6ba0d7075a8ef7f9680a91ae69d 
> 
> Diff: https://reviews.apache.org/r/42332/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Review Request 42332: Trigger shutdown on task pruning failure.

2016-01-14 Thread Zameer Manji 

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

Review request for Aurora, John Sirois and Maxim Khutornenko.


Bugs: AURORA-1582
https://issues.apache.org/jira/browse/AURORA-1582


Repository: aurora


Description
---

Task pruning is critical to the operation of a large cluster. Failure to prune 
tasks can lead to storage growing in unexpected ways leading to scheduling 
slowdown. This patch adds shutdown on task pruning failure to prevent the 
scheduler from entering an undefined state.


Diffs
-

  src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 
2064089937f5178b1413d386a312f4173a0e35fb 
  src/test/java/org/apache/aurora/scheduler/pruning/TaskHistoryPrunerTest.java 
295960f13693c6ba0d7075a8ef7f9680a91ae69d 

Diff: https://reviews.apache.org/r/42332/diff/


Testing
---

./gradlew build -Pq


Thanks,

Zameer Manji

Re: Review Request 42328: Add metric for counting uncaught exceptions in async executor.

2016-01-14 Thread Aurora ReviewBot 

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


Master (4dff5da) is green with this patch.
  ./build-support/jenkins/build.sh

However, it appears that it might lack test coverage.

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Jan. 14, 2016, 11:58 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42328/
> ---
> 
> (Updated Jan. 14, 2016, 11:58 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1582
> https://issues.apache.org/jira/browse/AURORA-1582
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add metric "async_executor_uncaught_exceptions" for tracking uncaught 
> exceptions in async executor.
> 
> 
> Diffs
> -
> 
>   NEWS 809077fe6f689e726204519f93fdabb89a4bb5e5 
>   src/main/java/org/apache/aurora/scheduler/base/AsyncUtil.java 
> 5facc048bc21adb124cb761647553afa9f8ab724 
> 
> Diff: https://reviews.apache.org/r/42328/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 42328: Add metric for counting uncaught exceptions in async executor.

2016-01-14 Thread John Sirois 

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

Ship it!


Ship It!

- John Sirois


On Jan. 14, 2016, 4:58 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42328/
> ---
> 
> (Updated Jan. 14, 2016, 4:58 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1582
> https://issues.apache.org/jira/browse/AURORA-1582
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add metric "async_executor_uncaught_exceptions" for tracking uncaught 
> exceptions in async executor.
> 
> 
> Diffs
> -
> 
>   NEWS 809077fe6f689e726204519f93fdabb89a4bb5e5 
>   src/main/java/org/apache/aurora/scheduler/base/AsyncUtil.java 
> 5facc048bc21adb124cb761647553afa9f8ab724 
> 
> Diff: https://reviews.apache.org/r/42328/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 42328: Add metric for counting uncaught exceptions in async executor.

2016-01-14 Thread Zameer Manji 

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

(Updated Jan. 14, 2016, 3:58 p.m.)


Review request for Aurora, John Sirois and Maxim Khutornenko.


Changes
---

John's feedback.


Bugs: AURORA-1582
https://issues.apache.org/jira/browse/AURORA-1582


Repository: aurora


Description
---

Add metric "async_executor_uncaught_exceptions" for tracking uncaught 
exceptions in async executor.


Diffs (updated)
-

  NEWS 809077fe6f689e726204519f93fdabb89a4bb5e5 
  src/main/java/org/apache/aurora/scheduler/base/AsyncUtil.java 
5facc048bc21adb124cb761647553afa9f8ab724 

Diff: https://reviews.apache.org/r/42328/diff/


Testing
---


Thanks,

Zameer Manji

Re: Review Request 42328: Add metric for counting uncaught exceptions in async executor.

2016-01-14 Thread Aurora ReviewBot 

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


Master (4dff5da) is green with this patch.
  ./build-support/jenkins/build.sh

However, it appears that it might lack test coverage.

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Jan. 14, 2016, 11:27 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42328/
> ---
> 
> (Updated Jan. 14, 2016, 11:27 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1582
> https://issues.apache.org/jira/browse/AURORA-1582
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add metric "async_executor_uncaught_exceptions" for tracking uncaught 
> exceptions in async executor.
> 
> 
> Diffs
> -
> 
>   NEWS 809077fe6f689e726204519f93fdabb89a4bb5e5 
>   src/main/java/org/apache/aurora/scheduler/base/AsyncUtil.java 
> 5facc048bc21adb124cb761647553afa9f8ab724 
> 
> Diff: https://reviews.apache.org/r/42328/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 42328: Add metric for counting uncaught exceptions in async executor.

2016-01-14 Thread John Sirois 

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

Ship it!



src/main/java/org/apache/aurora/scheduler/base/AsyncUtil.java (line 40)


final


- John Sirois


On Jan. 14, 2016, 4:27 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42328/
> ---
> 
> (Updated Jan. 14, 2016, 4:27 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1582
> https://issues.apache.org/jira/browse/AURORA-1582
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add metric "async_executor_uncaught_exceptions" for tracking uncaught 
> exceptions in async executor.
> 
> 
> Diffs
> -
> 
>   NEWS 809077fe6f689e726204519f93fdabb89a4bb5e5 
>   src/main/java/org/apache/aurora/scheduler/base/AsyncUtil.java 
> 5facc048bc21adb124cb761647553afa9f8ab724 
> 
> Diff: https://reviews.apache.org/r/42328/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Review Request 42328: Add metric for counting uncaught exceptions in async executor.

2016-01-14 Thread Zameer Manji 

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

Review request for Aurora, John Sirois and Maxim Khutornenko.


Bugs: AURORA-1582
https://issues.apache.org/jira/browse/AURORA-1582


Repository: aurora


Description
---

Add metric "async_executor_uncaught_exceptions" for tracking uncaught 
exceptions in async executor.


Diffs
-

  NEWS 809077fe6f689e726204519f93fdabb89a4bb5e5 
  src/main/java/org/apache/aurora/scheduler/base/AsyncUtil.java 
5facc048bc21adb124cb761647553afa9f8ab724 

Diff: https://reviews.apache.org/r/42328/diff/


Testing
---


Thanks,

Zameer Manji

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

2016-01-14 Thread Zameer Manji 

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


Amol, can you please rebase so I can apply this change?

- Zameer Manji


On Jan. 14, 2016, 1:10 p.m., Amol Deshmukh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42046/
> ---
> 
> (Updated Jan. 14, 2016, 1:10 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.
> 
> 
> Bugs: AURORA-1576
> https://issues.apache.org/jira/browse/AURORA-1576
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow for plugging in cli-configurable filters that are invoked post shiro 
> filters.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   docs/security.md f9b60e9afa3280dc132067c5167f624c34ade66f 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
>  4b6a872737e5934394e9511af7b6bd96c6034e72 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java
>  ac92117e14c105bec74e49d8e80eb56008237abf 
> 
> Diff: https://reviews.apache.org/r/42046/diff/
> 
> 
> Testing
> ---
> 
> Unit tests:
> ```
> $ ./gradlew clean test
> ...
> BUILD SUCCESSFUL
> 
> Total time: 3 mins 24.616 secs
> ```
> 
> End to end tests:
> ```
> $ ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> ...
> 
> *** OK (All tests passed) ***
> 
> aurora-scheduler-kerberos stop/waiting
> aurora-scheduler start/running, process 15362
> + RETCODE=0
> + restore_netrc
> + mv /home/vagrant/.netrc.bak /home/vagrant/.netrc
> + true
> Connection to 127.0.0.1 closed.
> 
> ```
> 
> 
> Thanks,
> 
> Amol Deshmukh
> 
>

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

2016-01-14 Thread Zameer Manji 

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

Ship it!


Ship It!

- Zameer Manji


On Jan. 14, 2016, 1:10 p.m., Amol Deshmukh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42046/
> ---
> 
> (Updated Jan. 14, 2016, 1:10 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.
> 
> 
> Bugs: AURORA-1576
> https://issues.apache.org/jira/browse/AURORA-1576
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow for plugging in cli-configurable filters that are invoked post shiro 
> filters.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   docs/security.md f9b60e9afa3280dc132067c5167f624c34ade66f 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
>  4b6a872737e5934394e9511af7b6bd96c6034e72 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java
>  ac92117e14c105bec74e49d8e80eb56008237abf 
> 
> Diff: https://reviews.apache.org/r/42046/diff/
> 
> 
> Testing
> ---
> 
> Unit tests:
> ```
> $ ./gradlew clean test
> ...
> BUILD SUCCESSFUL
> 
> Total time: 3 mins 24.616 secs
> ```
> 
> End to end tests:
> ```
> $ ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> ...
> 
> *** OK (All tests passed) ***
> 
> aurora-scheduler-kerberos stop/waiting
> aurora-scheduler start/running, process 15362
> + RETCODE=0
> + restore_netrc
> + mv /home/vagrant/.netrc.bak /home/vagrant/.netrc
> + true
> Connection to 127.0.0.1 closed.
> 
> ```
> 
> 
> Thanks,
> 
> Amol Deshmukh
> 
>

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

2016-01-14 Thread Aurora ReviewBot 

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

Ship it!


Master (a80260e) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Jan. 14, 2016, 9:10 p.m., Amol Deshmukh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42046/
> ---
> 
> (Updated Jan. 14, 2016, 9:10 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.
> 
> 
> Bugs: AURORA-1576
> https://issues.apache.org/jira/browse/AURORA-1576
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow for plugging in cli-configurable filters that are invoked post shiro 
> filters.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   docs/security.md f9b60e9afa3280dc132067c5167f624c34ade66f 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
>  4b6a872737e5934394e9511af7b6bd96c6034e72 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java
>  ac92117e14c105bec74e49d8e80eb56008237abf 
> 
> Diff: https://reviews.apache.org/r/42046/diff/
> 
> 
> Testing
> ---
> 
> Unit tests:
> ```
> $ ./gradlew clean test
> ...
> BUILD SUCCESSFUL
> 
> Total time: 3 mins 24.616 secs
> ```
> 
> End to end tests:
> ```
> $ ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> ...
> 
> *** OK (All tests passed) ***
> 
> aurora-scheduler-kerberos stop/waiting
> aurora-scheduler start/running, process 15362
> + RETCODE=0
> + restore_netrc
> + mv /home/vagrant/.netrc.bak /home/vagrant/.netrc
> + true
> Connection to 127.0.0.1 closed.
> 
> ```
> 
> 
> Thanks,
> 
> Amol Deshmukh
> 
>

Re: Review Request 42321: fix typo in the user guide about Task Updates

2016-01-14 Thread John Sirois 


> On Jan. 14, 2016, 2:24 p.m., John Sirois wrote:
> > Ship It!

Thanks for going from PR -> RB!


- John


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


On Jan. 14, 2016, 2:23 p.m., Anant Vyas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42321/
> ---
> 
> (Updated Jan. 14, 2016, 2:23 p.m.)
> 
> 
> Review request for Aurora.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> fix typo in the user guide about Task Updates
> 
> 
> Diffs
> -
> 
>   docs/user-guide.md 1a78d30dbeaa896e97a8946783d8b050e7f4b697 
> 
> Diff: https://reviews.apache.org/r/42321/diff/
> 
> 
> Testing
> ---
> 
> Fix a minor typo in the user guide and add a missing "and"
> 
> 
> Thanks,
> 
> Anant Vyas
> 
>

Re: Review Request 42321: fix typo in the user guide about Task Updates

2016-01-14 Thread John Sirois 

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

Ship it!


Ship It!

- John Sirois


On Jan. 14, 2016, 2:23 p.m., Anant Vyas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42321/
> ---
> 
> (Updated Jan. 14, 2016, 2:23 p.m.)
> 
> 
> Review request for Aurora.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> fix typo in the user guide about Task Updates
> 
> 
> Diffs
> -
> 
>   docs/user-guide.md 1a78d30dbeaa896e97a8946783d8b050e7f4b697 
> 
> Diff: https://reviews.apache.org/r/42321/diff/
> 
> 
> Testing
> ---
> 
> Fix a minor typo in the user guide and add a missing "and"
> 
> 
> Thanks,
> 
> Anant Vyas
> 
>

Review Request 42321: fix typo in the user guide about Task Updates

2016-01-14 Thread Anant Vyas 

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

Review request for Aurora.


Repository: aurora


Description
---

fix typo in the user guide about Task Updates


Diffs
-

  docs/user-guide.md 1a78d30dbeaa896e97a8946783d8b050e7f4b697 

Diff: https://reviews.apache.org/r/42321/diff/


Testing
---

Fix a minor typo in the user guide and add a missing "and"


Thanks,

Anant Vyas

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

2016-01-14 Thread Amol Deshmukh 

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

(Updated Jan. 14, 2016, 1:10 p.m.)


Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.


Changes
---

Implemented the last round of feedback from wfarner. In ``HttpSecurityIT``:
1. bound the Shiro post auth filter using an annotation
2. replaced use of EasyMock captures with ``EasyMock.getCurrentArguments`` for 
implementing the pass-through filter behavior.


Bugs: AURORA-1576
https://issues.apache.org/jira/browse/AURORA-1576


Repository: aurora


Description
---

Allow for plugging in cli-configurable filters that are invoked post shiro 
filters.


Diffs (updated)
-

  NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
  docs/security.md f9b60e9afa3280dc132067c5167f624c34ade66f 
  
src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
 4b6a872737e5934394e9511af7b6bd96c6034e72 
  
src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java 
ac92117e14c105bec74e49d8e80eb56008237abf 

Diff: https://reviews.apache.org/r/42046/diff/


Testing
---

Unit tests:
```
$ ./gradlew clean test
...
BUILD SUCCESSFUL

Total time: 3 mins 24.616 secs
```

End to end tests:
```
$ ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
...

*** OK (All tests passed) ***

aurora-scheduler-kerberos stop/waiting
aurora-scheduler start/running, process 15362
+ RETCODE=0
+ restore_netrc
+ mv /home/vagrant/.netrc.bak /home/vagrant/.netrc
+ true
Connection to 127.0.0.1 closed.

```


Thanks,

Amol Deshmukh

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

2016-01-14 Thread Amol Deshmukh 


> On Jan. 14, 2016, 12:29 p.m., Bill Farner wrote:
> > One more drive-by: in the test I don't believe you need captures.  EasyMock 
> > gives you access to call arguments (EasyMock.getCurrentArguments I believe) 
> > within the andAnswer.  This unfortunately requires you to cast, but I find 
> > it preferable as the resulting code is more self-contained.

Thanks, I was not aware of ``EasyMock.getCurrentArguments``.


- Amol


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


On Jan. 14, 2016, 11:32 a.m., Amol Deshmukh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42046/
> ---
> 
> (Updated Jan. 14, 2016, 11:32 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.
> 
> 
> Bugs: AURORA-1576
> https://issues.apache.org/jira/browse/AURORA-1576
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow for plugging in cli-configurable filters that are invoked post shiro 
> filters.
> 
> 
> Diffs
> -
> 
>   NEWS ecb26d2af2c624042e7819acde004278520b3cae 
>   docs/security.md f9b60e9afa3280dc132067c5167f624c34ade66f 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
>  4b6a872737e5934394e9511af7b6bd96c6034e72 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java
>  ac92117e14c105bec74e49d8e80eb56008237abf 
> 
> Diff: https://reviews.apache.org/r/42046/diff/
> 
> 
> Testing
> ---
> 
> Unit tests:
> ```
> $ ./gradlew clean test
> ...
> BUILD SUCCESSFUL
> 
> Total time: 3 mins 24.616 secs
> ```
> 
> End to end tests:
> ```
> $ ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> ...
> 
> *** OK (All tests passed) ***
> 
> aurora-scheduler-kerberos stop/waiting
> aurora-scheduler start/running, process 15362
> + RETCODE=0
> + restore_netrc
> + mv /home/vagrant/.netrc.bak /home/vagrant/.netrc
> + true
> Connection to 127.0.0.1 closed.
> 
> ```
> 
> 
> Thanks,
> 
> Amol Deshmukh
> 
>

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

2016-01-14 Thread Amol Deshmukh 


> On Jan. 14, 2016, 12:26 p.m., Bill Farner wrote:
> > On mobile and can't comment on the specific line, but you should qualify 
> > the `Filter` binding with a binding annotation.  This is best practice for 
> > any binding that is moderately generic.

Ack.


- Amol


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


On Jan. 14, 2016, 11:32 a.m., Amol Deshmukh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42046/
> ---
> 
> (Updated Jan. 14, 2016, 11:32 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.
> 
> 
> Bugs: AURORA-1576
> https://issues.apache.org/jira/browse/AURORA-1576
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow for plugging in cli-configurable filters that are invoked post shiro 
> filters.
> 
> 
> Diffs
> -
> 
>   NEWS ecb26d2af2c624042e7819acde004278520b3cae 
>   docs/security.md f9b60e9afa3280dc132067c5167f624c34ade66f 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
>  4b6a872737e5934394e9511af7b6bd96c6034e72 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java
>  ac92117e14c105bec74e49d8e80eb56008237abf 
> 
> Diff: https://reviews.apache.org/r/42046/diff/
> 
> 
> Testing
> ---
> 
> Unit tests:
> ```
> $ ./gradlew clean test
> ...
> BUILD SUCCESSFUL
> 
> Total time: 3 mins 24.616 secs
> ```
> 
> End to end tests:
> ```
> $ ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> ...
> 
> *** OK (All tests passed) ***
> 
> aurora-scheduler-kerberos stop/waiting
> aurora-scheduler start/running, process 15362
> + RETCODE=0
> + restore_netrc
> + mv /home/vagrant/.netrc.bak /home/vagrant/.netrc
> + true
> Connection to 127.0.0.1 closed.
> 
> ```
> 
> 
> Thanks,
> 
> Amol Deshmukh
> 
>

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

2016-01-14 Thread Bill Farner 

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


One more drive-by: in the test I don't believe you need captures.  EasyMock 
gives you access to call arguments (EasyMock.getCurrentArguments I believe) 
within the andAnswer.  This unfortunately requires you to cast, but I find it 
preferable as the resulting code is more self-contained.

- Bill Farner


On Jan. 14, 2016, 11:32 a.m., Amol Deshmukh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42046/
> ---
> 
> (Updated Jan. 14, 2016, 11:32 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.
> 
> 
> Bugs: AURORA-1576
> https://issues.apache.org/jira/browse/AURORA-1576
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow for plugging in cli-configurable filters that are invoked post shiro 
> filters.
> 
> 
> Diffs
> -
> 
>   NEWS ecb26d2af2c624042e7819acde004278520b3cae 
>   docs/security.md f9b60e9afa3280dc132067c5167f624c34ade66f 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
>  4b6a872737e5934394e9511af7b6bd96c6034e72 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java
>  ac92117e14c105bec74e49d8e80eb56008237abf 
> 
> Diff: https://reviews.apache.org/r/42046/diff/
> 
> 
> Testing
> ---
> 
> Unit tests:
> ```
> $ ./gradlew clean test
> ...
> BUILD SUCCESSFUL
> 
> Total time: 3 mins 24.616 secs
> ```
> 
> End to end tests:
> ```
> $ ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> ...
> 
> *** OK (All tests passed) ***
> 
> aurora-scheduler-kerberos stop/waiting
> aurora-scheduler start/running, process 15362
> + RETCODE=0
> + restore_netrc
> + mv /home/vagrant/.netrc.bak /home/vagrant/.netrc
> + true
> Connection to 127.0.0.1 closed.
> 
> ```
> 
> 
> Thanks,
> 
> Amol Deshmukh
> 
>

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

2016-01-14 Thread Bill Farner 

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


On mobile and can't comment on the specific line, but you should qualify the 
`Filter` binding with a binding annotation.  This is best practice for any 
binding that is moderately generic.

- Bill Farner


On Jan. 14, 2016, 11:32 a.m., Amol Deshmukh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42046/
> ---
> 
> (Updated Jan. 14, 2016, 11:32 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.
> 
> 
> Bugs: AURORA-1576
> https://issues.apache.org/jira/browse/AURORA-1576
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow for plugging in cli-configurable filters that are invoked post shiro 
> filters.
> 
> 
> Diffs
> -
> 
>   NEWS ecb26d2af2c624042e7819acde004278520b3cae 
>   docs/security.md f9b60e9afa3280dc132067c5167f624c34ade66f 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
>  4b6a872737e5934394e9511af7b6bd96c6034e72 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java
>  ac92117e14c105bec74e49d8e80eb56008237abf 
> 
> Diff: https://reviews.apache.org/r/42046/diff/
> 
> 
> Testing
> ---
> 
> Unit tests:
> ```
> $ ./gradlew clean test
> ...
> BUILD SUCCESSFUL
> 
> Total time: 3 mins 24.616 secs
> ```
> 
> End to end tests:
> ```
> $ ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> ...
> 
> *** OK (All tests passed) ***
> 
> aurora-scheduler-kerberos stop/waiting
> aurora-scheduler start/running, process 15362
> + RETCODE=0
> + restore_netrc
> + mv /home/vagrant/.netrc.bak /home/vagrant/.netrc
> + true
> Connection to 127.0.0.1 closed.
> 
> ```
> 
> 
> Thanks,
> 
> Amol Deshmukh
> 
>

Re: Review Request 42126: Accept resource offers from multiple framework roles.

2016-01-14 Thread Dmitriy Shirchenko 

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

Ship it!


Ship It!

- Dmitriy Shirchenko


On Jan. 14, 2016, 6:23 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 14, 2016, 6:23 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
> Farner.
> 
> 
> Bugs: AURORA-1109
> https://issues.apache.org/jira/browse/AURORA-1109
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This review is a prototype for introducing multiple role support in Aurora.
> This creates a new class OfferAllocation, which allcoates resources to 
> resources field in TaskInfo and ExecutorInfo from an offer.
> 
> Current implementation prefers reserved resources over shared resources ('*' 
> role) if both are present
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/ResourceSlot.java 
> 7c3d681c216b78eeecebbe950186e5a79c6fe982 
>   src/main/java/org/apache/aurora/scheduler/Resources.java 
> db422a959ee7b982c2a44323de41ad75d1a40754 
>   
> src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java
>  2255dd407cd1810c7df5baf17cfa85f79bfffeb8 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> 8fdadda67478bb3110aa442b7d78493cf9c3edb4 
>   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
> 7e8e456e288986eb0ce92a123b294e1e25d8ed18 
>   src/test/java/org/apache/aurora/scheduler/AcceptedOfferTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 
> e4ae943303823ac4bfbe999ed22f5999484462d8 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModuleTest.java
>  33149ab415292eff04f38b61f2b1d1eac79f347a 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
> a5793bffabf4e5d6195b1b99f2363d241c0cecf9 
>   src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
> 3cbe9acd75def14ae2e0986914ba621fb164b3e4 
> 
> Diff: https://reviews.apache.org/r/42126/diff/
> 
> 
> Testing
> ---
> 
> 1. Unit tested with old and new tests;
> 2. vagrant integration tests: I manually separate out the vagrant box's cpu 
> and memory between 'aurora-test' role and '*' and verified that jobs can 
> still be launched (I can post the vagrant change in another follow upon 
> request).
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 42126: Accept resource offers from multiple framework roles.

2016-01-14 Thread Aurora ReviewBot 

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

Ship it!


Master (952ef6d) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Jan. 14, 2016, 6:23 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 14, 2016, 6:23 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
> Farner.
> 
> 
> Bugs: AURORA-1109
> https://issues.apache.org/jira/browse/AURORA-1109
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This review is a prototype for introducing multiple role support in Aurora.
> This creates a new class OfferAllocation, which allcoates resources to 
> resources field in TaskInfo and ExecutorInfo from an offer.
> 
> Current implementation prefers reserved resources over shared resources ('*' 
> role) if both are present
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/ResourceSlot.java 
> 7c3d681c216b78eeecebbe950186e5a79c6fe982 
>   src/main/java/org/apache/aurora/scheduler/Resources.java 
> db422a959ee7b982c2a44323de41ad75d1a40754 
>   
> src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java
>  2255dd407cd1810c7df5baf17cfa85f79bfffeb8 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> 8fdadda67478bb3110aa442b7d78493cf9c3edb4 
>   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
> 7e8e456e288986eb0ce92a123b294e1e25d8ed18 
>   src/test/java/org/apache/aurora/scheduler/AcceptedOfferTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 
> e4ae943303823ac4bfbe999ed22f5999484462d8 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModuleTest.java
>  33149ab415292eff04f38b61f2b1d1eac79f347a 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
> a5793bffabf4e5d6195b1b99f2363d241c0cecf9 
>   src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
> 3cbe9acd75def14ae2e0986914ba621fb164b3e4 
> 
> Diff: https://reviews.apache.org/r/42126/diff/
> 
> 
> Testing
> ---
> 
> 1. Unit tested with old and new tests;
> 2. vagrant integration tests: I manually separate out the vagrant box's cpu 
> and memory between 'aurora-test' role and '*' and verified that jobs can 
> still be launched (I can post the vagrant change in another follow upon 
> request).
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 42126: Accept resource offers from multiple framework roles.

2016-01-14 Thread Maxim Khutornenko 


> On Jan. 14, 2016, 6:31 p.m., Maxim Khutornenko wrote:
> > Ship It!

Waiting for Dmitriy to sign off before committing it to master.


- Maxim


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


On Jan. 14, 2016, 6:23 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 14, 2016, 6:23 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
> Farner.
> 
> 
> Bugs: AURORA-1109
> https://issues.apache.org/jira/browse/AURORA-1109
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This review is a prototype for introducing multiple role support in Aurora.
> This creates a new class OfferAllocation, which allcoates resources to 
> resources field in TaskInfo and ExecutorInfo from an offer.
> 
> Current implementation prefers reserved resources over shared resources ('*' 
> role) if both are present
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/ResourceSlot.java 
> 7c3d681c216b78eeecebbe950186e5a79c6fe982 
>   src/main/java/org/apache/aurora/scheduler/Resources.java 
> db422a959ee7b982c2a44323de41ad75d1a40754 
>   
> src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java
>  2255dd407cd1810c7df5baf17cfa85f79bfffeb8 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> 8fdadda67478bb3110aa442b7d78493cf9c3edb4 
>   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
> 7e8e456e288986eb0ce92a123b294e1e25d8ed18 
>   src/test/java/org/apache/aurora/scheduler/AcceptedOfferTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 
> e4ae943303823ac4bfbe999ed22f5999484462d8 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModuleTest.java
>  33149ab415292eff04f38b61f2b1d1eac79f347a 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
> a5793bffabf4e5d6195b1b99f2363d241c0cecf9 
>   src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
> 3cbe9acd75def14ae2e0986914ba621fb164b3e4 
> 
> Diff: https://reviews.apache.org/r/42126/diff/
> 
> 
> Testing
> ---
> 
> 1. Unit tested with old and new tests;
> 2. vagrant integration tests: I manually separate out the vagrant box's cpu 
> and memory between 'aurora-test' role and '*' and verified that jobs can 
> still be launched (I can post the vagrant change in another follow upon 
> request).
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 42126: Accept resource offers from multiple framework roles.

2016-01-14 Thread Maxim Khutornenko 

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

Ship it!


Ship It!

- Maxim Khutornenko


On Jan. 14, 2016, 6:23 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 14, 2016, 6:23 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
> Farner.
> 
> 
> Bugs: AURORA-1109
> https://issues.apache.org/jira/browse/AURORA-1109
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This review is a prototype for introducing multiple role support in Aurora.
> This creates a new class OfferAllocation, which allcoates resources to 
> resources field in TaskInfo and ExecutorInfo from an offer.
> 
> Current implementation prefers reserved resources over shared resources ('*' 
> role) if both are present
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/ResourceSlot.java 
> 7c3d681c216b78eeecebbe950186e5a79c6fe982 
>   src/main/java/org/apache/aurora/scheduler/Resources.java 
> db422a959ee7b982c2a44323de41ad75d1a40754 
>   
> src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java
>  2255dd407cd1810c7df5baf17cfa85f79bfffeb8 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> 8fdadda67478bb3110aa442b7d78493cf9c3edb4 
>   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
> 7e8e456e288986eb0ce92a123b294e1e25d8ed18 
>   src/test/java/org/apache/aurora/scheduler/AcceptedOfferTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 
> e4ae943303823ac4bfbe999ed22f5999484462d8 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModuleTest.java
>  33149ab415292eff04f38b61f2b1d1eac79f347a 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
> a5793bffabf4e5d6195b1b99f2363d241c0cecf9 
>   src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
> 3cbe9acd75def14ae2e0986914ba621fb164b3e4 
> 
> Diff: https://reviews.apache.org/r/42126/diff/
> 
> 
> Testing
> ---
> 
> 1. Unit tested with old and new tests;
> 2. vagrant integration tests: I manually separate out the vagrant box's cpu 
> and memory between 'aurora-test' role and '*' and verified that jobs can 
> still be launched (I can post the vagrant change in another follow upon 
> request).
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 42126: Accept resource offers from multiple framework roles.

2016-01-14 Thread Zhitao Li 


> On Jan. 14, 2016, 6:14 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java,
> >  line 95
> > 
> >
> > This is still not technically correct. If role is not set Mesos will 
> > not send role with resources at all. All resources will be with role=Null 
> > in that case. I'd just drop (role="*") from help.

Fixed.


- Zhitao


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


On Jan. 14, 2016, 6:23 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 14, 2016, 6:23 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
> Farner.
> 
> 
> Bugs: AURORA-1109
> https://issues.apache.org/jira/browse/AURORA-1109
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This review is a prototype for introducing multiple role support in Aurora.
> This creates a new class OfferAllocation, which allcoates resources to 
> resources field in TaskInfo and ExecutorInfo from an offer.
> 
> Current implementation prefers reserved resources over shared resources ('*' 
> role) if both are present
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/ResourceSlot.java 
> 7c3d681c216b78eeecebbe950186e5a79c6fe982 
>   src/main/java/org/apache/aurora/scheduler/Resources.java 
> db422a959ee7b982c2a44323de41ad75d1a40754 
>   
> src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java
>  2255dd407cd1810c7df5baf17cfa85f79bfffeb8 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> 8fdadda67478bb3110aa442b7d78493cf9c3edb4 
>   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
> 7e8e456e288986eb0ce92a123b294e1e25d8ed18 
>   src/test/java/org/apache/aurora/scheduler/AcceptedOfferTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 
> e4ae943303823ac4bfbe999ed22f5999484462d8 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModuleTest.java
>  33149ab415292eff04f38b61f2b1d1eac79f347a 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
> a5793bffabf4e5d6195b1b99f2363d241c0cecf9 
>   src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
> 3cbe9acd75def14ae2e0986914ba621fb164b3e4 
> 
> Diff: https://reviews.apache.org/r/42126/diff/
> 
> 
> Testing
> ---
> 
> 1. Unit tested with old and new tests;
> 2. vagrant integration tests: I manually separate out the vagrant box's cpu 
> and memory between 'aurora-test' role and '*' and verified that jobs can 
> still be launched (I can post the vagrant change in another follow upon 
> request).
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 42126: Accept resource offers from multiple framework roles.

2016-01-14 Thread Zhitao Li 

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

(Updated Jan. 14, 2016, 6:23 p.m.)


Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
Farner.


Changes
---

Clearer helper message as requested by Maxim


Bugs: AURORA-1109
https://issues.apache.org/jira/browse/AURORA-1109


Repository: aurora


Description
---

This review is a prototype for introducing multiple role support in Aurora.
This creates a new class OfferAllocation, which allcoates resources to 
resources field in TaskInfo and ExecutorInfo from an offer.

Current implementation prefers reserved resources over shared resources ('*' 
role) if both are present

Several caveats:
1. This performs the allocate after scheduling decision in 
TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency and 
late failure.


Diffs (updated)
-

  NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
  src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/ResourceSlot.java 
7c3d681c216b78eeecebbe950186e5a79c6fe982 
  src/main/java/org/apache/aurora/scheduler/Resources.java 
db422a959ee7b982c2a44323de41ad75d1a40754 
  
src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java
 2255dd407cd1810c7df5baf17cfa85f79bfffeb8 
  src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
8fdadda67478bb3110aa442b7d78493cf9c3edb4 
  src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
7e8e456e288986eb0ce92a123b294e1e25d8ed18 
  src/test/java/org/apache/aurora/scheduler/AcceptedOfferTest.java PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 
e4ae943303823ac4bfbe999ed22f5999484462d8 
  
src/test/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModuleTest.java
 33149ab415292eff04f38b61f2b1d1eac79f347a 
  src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
a5793bffabf4e5d6195b1b99f2363d241c0cecf9 
  src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
3cbe9acd75def14ae2e0986914ba621fb164b3e4 

Diff: https://reviews.apache.org/r/42126/diff/


Testing
---

1. Unit tested with old and new tests;
2. vagrant integration tests: I manually separate out the vagrant box's cpu and 
memory between 'aurora-test' role and '*' and verified that jobs can still be 
launched (I can post the vagrant change in another follow upon request).


Thanks,

Zhitao Li

Re: Review Request 42126: Accept resource offers from multiple framework roles.

2016-01-14 Thread Maxim Khutornenko 

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



src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java
 (line 95)


This is still not technically correct. If role is not set Mesos will not 
send role with resources at all. All resources will be with role=Null in that 
case. I'd just drop (role="*") from help.


- Maxim Khutornenko


On Jan. 14, 2016, 4:32 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 14, 2016, 4:32 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
> Farner.
> 
> 
> Bugs: AURORA-1109
> https://issues.apache.org/jira/browse/AURORA-1109
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This review is a prototype for introducing multiple role support in Aurora.
> This creates a new class OfferAllocation, which allcoates resources to 
> resources field in TaskInfo and ExecutorInfo from an offer.
> 
> Current implementation prefers reserved resources over shared resources ('*' 
> role) if both are present
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/ResourceSlot.java 
> 7c3d681c216b78eeecebbe950186e5a79c6fe982 
>   src/main/java/org/apache/aurora/scheduler/Resources.java 
> db422a959ee7b982c2a44323de41ad75d1a40754 
>   
> src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java
>  2255dd407cd1810c7df5baf17cfa85f79bfffeb8 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> 8fdadda67478bb3110aa442b7d78493cf9c3edb4 
>   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
> 7e8e456e288986eb0ce92a123b294e1e25d8ed18 
>   src/test/java/org/apache/aurora/scheduler/AcceptedOfferTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 
> e4ae943303823ac4bfbe999ed22f5999484462d8 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModuleTest.java
>  33149ab415292eff04f38b61f2b1d1eac79f347a 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
> a5793bffabf4e5d6195b1b99f2363d241c0cecf9 
>   src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
> 3cbe9acd75def14ae2e0986914ba621fb164b3e4 
> 
> Diff: https://reviews.apache.org/r/42126/diff/
> 
> 
> Testing
> ---
> 
> 1. Unit tested with old and new tests;
> 2. vagrant integration tests: I manually separate out the vagrant box's cpu 
> and memory between 'aurora-test' role and '*' and verified that jobs can 
> still be launched (I can post the vagrant change in another follow upon 
> request).
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 42126: Accept resource offers from multiple framework roles.

2016-01-14 Thread Aurora ReviewBot 

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

Ship it!


Master (952ef6d) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Jan. 14, 2016, 4:32 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 14, 2016, 4:32 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
> Farner.
> 
> 
> Bugs: AURORA-1109
> https://issues.apache.org/jira/browse/AURORA-1109
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This review is a prototype for introducing multiple role support in Aurora.
> This creates a new class OfferAllocation, which allcoates resources to 
> resources field in TaskInfo and ExecutorInfo from an offer.
> 
> Current implementation prefers reserved resources over shared resources ('*' 
> role) if both are present
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/ResourceSlot.java 
> 7c3d681c216b78eeecebbe950186e5a79c6fe982 
>   src/main/java/org/apache/aurora/scheduler/Resources.java 
> db422a959ee7b982c2a44323de41ad75d1a40754 
>   
> src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java
>  2255dd407cd1810c7df5baf17cfa85f79bfffeb8 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> 8fdadda67478bb3110aa442b7d78493cf9c3edb4 
>   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
> 7e8e456e288986eb0ce92a123b294e1e25d8ed18 
>   src/test/java/org/apache/aurora/scheduler/AcceptedOfferTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 
> e4ae943303823ac4bfbe999ed22f5999484462d8 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModuleTest.java
>  33149ab415292eff04f38b61f2b1d1eac79f347a 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
> a5793bffabf4e5d6195b1b99f2363d241c0cecf9 
>   src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
> 3cbe9acd75def14ae2e0986914ba621fb164b3e4 
> 
> Diff: https://reviews.apache.org/r/42126/diff/
> 
> 
> Testing
> ---
> 
> 1. Unit tested with old and new tests;
> 2. vagrant integration tests: I manually separate out the vagrant box's cpu 
> and memory between 'aurora-test' role and '*' and verified that jobs can 
> still be launched (I can post the vagrant change in another follow upon 
> request).
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 42126: Accept resource offers from multiple framework roles.

2016-01-14 Thread Zhitao Li 

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

(Updated Jan. 14, 2016, 4:32 p.m.)


Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
Farner.


Changes
---

Use a `nearZero()` function to avoid comparison floating point number with 
zero, and restore a test case using `SOME_OVERHEAD_EXECUTOR`


Bugs: AURORA-1109
https://issues.apache.org/jira/browse/AURORA-1109


Repository: aurora


Description
---

This review is a prototype for introducing multiple role support in Aurora.
This creates a new class OfferAllocation, which allcoates resources to 
resources field in TaskInfo and ExecutorInfo from an offer.

Current implementation prefers reserved resources over shared resources ('*' 
role) if both are present

Several caveats:
1. This performs the allocate after scheduling decision in 
TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency and 
late failure.


Diffs (updated)
-

  NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
  src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/ResourceSlot.java 
7c3d681c216b78eeecebbe950186e5a79c6fe982 
  src/main/java/org/apache/aurora/scheduler/Resources.java 
db422a959ee7b982c2a44323de41ad75d1a40754 
  
src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java
 2255dd407cd1810c7df5baf17cfa85f79bfffeb8 
  src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
8fdadda67478bb3110aa442b7d78493cf9c3edb4 
  src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
7e8e456e288986eb0ce92a123b294e1e25d8ed18 
  src/test/java/org/apache/aurora/scheduler/AcceptedOfferTest.java PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 
e4ae943303823ac4bfbe999ed22f5999484462d8 
  
src/test/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModuleTest.java
 33149ab415292eff04f38b61f2b1d1eac79f347a 
  src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
a5793bffabf4e5d6195b1b99f2363d241c0cecf9 
  src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
3cbe9acd75def14ae2e0986914ba621fb164b3e4 

Diff: https://reviews.apache.org/r/42126/diff/


Testing
---

1. Unit tested with old and new tests;
2. vagrant integration tests: I manually separate out the vagrant box's cpu and 
memory between 'aurora-test' role and '*' and verified that jobs can still be 
launched (I can post the vagrant change in another follow upon request).


Thanks,

Zhitao Li

Re: Review Request 42126: Accept resource offers from multiple framework roles.

2016-01-14 Thread Zhitao Li 


> On Jan. 14, 2016, 1:06 a.m., Dmitriy Shirchenko wrote:
> > src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java,
> >  lines 216-217
> > 
> >
> > Is this intentionally commented out?
> 
> Zhitao Li wrote:
> Yes this is intentially removed. Old behavior of changing this `config` 
> here resulted in `resources` in static final object `OFFER` not sufficient to 
> be allocated, and I don't see any point to use a different executor for this 
> test case, so I've deleted this code.
> 
> If any reviewer thinks there is a reason to test this with a different 
> executor, I can reconstruct an `Offer` object with correct resource and add 
> this back.
> 
> Bill Farner wrote:
> This is slightly suspicious...are you sure resource accounting wasn't 
> impacted in some way?  I would expect old test cases to logically work with 
> this feature.
> 
> Zhitao Li wrote:
> I recalled why I commented this out in the first pass now (this was a bit 
> tricky and I mistakenly thought it's logical bug while it's due to floating 
> point precision).
> 
> In TaskExecutors.java, `SOME_OVERHEAD_EXECUTOR` uses `0.01` cpus:
> ```
>   public static final ExecutorSettings SOME_OVERHEAD_EXECUTOR =
>   TestExecutorSettings.thermosOnlyWithOverhead(
>   new ResourceSlot(0.01, Amount.of(256L, Data.MB), Amount.of(0L, 
> Data.MB), 0));
> ```
> Unfortunately, `5.01 - 5 - 0.01` **is not zero** in floating point math 
> of Java, so the test keeps throwing `InsufficientResourcesException` even if 
> I constructed an `Offer` whose resource is correctly equivalent to 
> `ResourceSlot.from(TASK_CONFIG).add(TaskExecutors.SOME_OVERHEAD_EXECUTOR.getExecutorOverhead())`.
> 
> **My proposal is to change all comparison with zero in `AcceptedOffer` 
> class (3 places in current patch) to compare with an `EPSILON` whose value is 
> `1e-6` or something even smaller.** I don't think there is any reasonable 
> usage of resource unit smaller than this granularity, and the floating point 
> math in the class would be more robust. We can document this behavior 
> somewhere if it sounds interesting.
> 
> The default resource usage for `THERMOS_EXECUTOR` is 0.25 so I don't 
> encounter this elsewhere.
> 
> Some alternatives which I liked less:
> 1. Change `TaskExecutors.SOME_OVERHEAD_EXECUTOR`'s cpus to a value which 
> could be precisely represented by floating point (0.125 or something similar);
> 2. Manually add a bit more cpu resource to the `Offer` object to trick 
> the test to pass.
> 
> If the above proposed solution sounds fine, I can send an update to the 
> patch.
> 
> Bill Farner wrote:
> I'm in favor of using an EPSILON value.  Please be sure to extract a 
> function to handle the equivalence check.
> 
> It's too bad mesos doesn't use fixed point for scalar resources to avoid 
> these issues.  Looks like there's been recent momentum to make that change: 
> https://issues.apache.org/jira/browse/MESOS-3997
> 
> They recently addressed an instance of this issue with this change:
> 
> https://github.com/apache/mesos/commit/5b5dd566aea71f654c51b2706a958ca1b5cb07a3
> 
> ```
> CHECK_NEAR(result.cpus().get(), cpus().get(), MIN_CPUS);
> ```
> 
> For reference, `MIN_CPUS` is 0.01.

Will do with a `nearZero()` helper function.

For the value of `ESPILON`, I took a quick look at related Mesos code, and the 
minimum gradunalar which could make a difference is `MIN_CPUS` / 
`CPU_SHARES_PER_CPU` (1024) in cgroup based isolation, which is still larger 
than `1e-6`, so using the latter value should ensure Aurora user won't lose 
precision even at extreme scheduling condition.


- Zhitao


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


On Jan. 14, 2016, 2:46 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 14, 2016, 2:46 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
> Farner.
> 
> 
> Bugs: AURORA-1109
> https://issues.apache.org/jira/browse/AURORA-1109
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This review is a prototype for introducing multiple role support in Aurora.
> This creates a new class OfferAllocation, which allcoates resources to 
> resources field in TaskInfo and ExecutorInfo from an offer.
> 
> Current implementation prefers reserved resources over shared resources ('*' 
> role) if both are present
> 
> Several caveats:
> 1. This performs the

Re: Review Request 42126: Accept resource offers from multiple framework roles.

2016-01-14 Thread Zhitao Li 


> On Jan. 14, 2016, 2:45 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java,
> >  line 216
> > 
> >
> > I assume this line will be restored once `AcceptedOffer` accommodates 
> > for floating point errors?

Yes, with a slightly different `Offer` object which uses `ResourceSlot` from 
`SOME_OVERHEAD_EXECUTOR` instead of `THERMOS_EXECUTOR`.


- Zhitao


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


On Jan. 14, 2016, 2:46 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 14, 2016, 2:46 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
> Farner.
> 
> 
> Bugs: AURORA-1109
> https://issues.apache.org/jira/browse/AURORA-1109
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This review is a prototype for introducing multiple role support in Aurora.
> This creates a new class OfferAllocation, which allcoates resources to 
> resources field in TaskInfo and ExecutorInfo from an offer.
> 
> Current implementation prefers reserved resources over shared resources ('*' 
> role) if both are present
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/ResourceSlot.java 
> 7c3d681c216b78eeecebbe950186e5a79c6fe982 
>   src/main/java/org/apache/aurora/scheduler/Resources.java 
> db422a959ee7b982c2a44323de41ad75d1a40754 
>   
> src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java
>  2255dd407cd1810c7df5baf17cfa85f79bfffeb8 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> 8fdadda67478bb3110aa442b7d78493cf9c3edb4 
>   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
> 7e8e456e288986eb0ce92a123b294e1e25d8ed18 
>   src/test/java/org/apache/aurora/scheduler/AcceptedOfferTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 
> e4ae943303823ac4bfbe999ed22f5999484462d8 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModuleTest.java
>  33149ab415292eff04f38b61f2b1d1eac79f347a 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
> a5793bffabf4e5d6195b1b99f2363d241c0cecf9 
>   src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
> 3cbe9acd75def14ae2e0986914ba621fb164b3e4 
> 
> Diff: https://reviews.apache.org/r/42126/diff/
> 
> 
> Testing
> ---
> 
> 1. Unit tested with old and new tests;
> 2. vagrant integration tests: I manually separate out the vagrant box's cpu 
> and memory between 'aurora-test' role and '*' and verified that jobs can 
> still be launched (I can post the vagrant change in another follow upon 
> request).
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 42126: Accept resource offers from multiple framework roles.

2016-01-14 Thread Zhitao Li 

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

(Updated Jan. 14, 2016, 6:46 a.m.)


Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
Farner.


Changes
---

Tweaked review summary in preparation for commit.

-wfarner


Summary (updated)
-

Accept resource offers from multiple framework roles.


Bugs: AURORA-1109
https://issues.apache.org/jira/browse/AURORA-1109


Repository: aurora


Description
---

This review is a prototype for introducing multiple role support in Aurora.
This creates a new class OfferAllocation, which allcoates resources to 
resources field in TaskInfo and ExecutorInfo from an offer.

Current implementation prefers reserved resources over shared resources ('*' 
role) if both are present

Several caveats:
1. This performs the allocate after scheduling decision in 
TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency and 
late failure.


Diffs
-

  NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
  src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/ResourceSlot.java 
7c3d681c216b78eeecebbe950186e5a79c6fe982 
  src/main/java/org/apache/aurora/scheduler/Resources.java 
db422a959ee7b982c2a44323de41ad75d1a40754 
  
src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java
 2255dd407cd1810c7df5baf17cfa85f79bfffeb8 
  src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
8fdadda67478bb3110aa442b7d78493cf9c3edb4 
  src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
7e8e456e288986eb0ce92a123b294e1e25d8ed18 
  src/test/java/org/apache/aurora/scheduler/AcceptedOfferTest.java PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 
e4ae943303823ac4bfbe999ed22f5999484462d8 
  
src/test/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModuleTest.java
 33149ab415292eff04f38b61f2b1d1eac79f347a 
  src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
a5793bffabf4e5d6195b1b99f2363d241c0cecf9 
  src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
3cbe9acd75def14ae2e0986914ba621fb164b3e4 

Diff: https://reviews.apache.org/r/42126/diff/


Testing
---

1. Unit tested with old and new tests;
2. vagrant integration tests: I manually separate out the vagrant box's cpu and 
memory between 'aurora-test' role and '*' and verified that jobs can still be 
launched (I can post the vagrant change in another follow upon request).


Thanks,

Zhitao Li

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

2016-01-14 Thread Bill Farner 

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



src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 


I assume this line will be restored once `AcceptedOffer` accommodates for 
floating point errors?


- Bill Farner


On Jan. 13, 2016, 5:39 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 13, 2016, 5:39 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
> Farner.
> 
> 
> Bugs: AURORA-1109
> https://issues.apache.org/jira/browse/AURORA-1109
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This review is a prototype for introducing multiple role support in Aurora.
> This creates a new class OfferAllocation, which allcoates resources to 
> resources field in TaskInfo and ExecutorInfo from an offer.
> 
> Current implementation prefers reserved resources over shared resources ('*' 
> role) if both are present
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/ResourceSlot.java 
> 7c3d681c216b78eeecebbe950186e5a79c6fe982 
>   src/main/java/org/apache/aurora/scheduler/Resources.java 
> db422a959ee7b982c2a44323de41ad75d1a40754 
>   
> src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java
>  2255dd407cd1810c7df5baf17cfa85f79bfffeb8 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> 8fdadda67478bb3110aa442b7d78493cf9c3edb4 
>   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
> 7e8e456e288986eb0ce92a123b294e1e25d8ed18 
>   src/test/java/org/apache/aurora/scheduler/AcceptedOfferTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 
> e4ae943303823ac4bfbe999ed22f5999484462d8 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModuleTest.java
>  33149ab415292eff04f38b61f2b1d1eac79f347a 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
> a5793bffabf4e5d6195b1b99f2363d241c0cecf9 
>   src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
> 3cbe9acd75def14ae2e0986914ba621fb164b3e4 
> 
> Diff: https://reviews.apache.org/r/42126/diff/
> 
> 
> Testing
> ---
> 
> 1. Unit tested with old and new tests;
> 2. vagrant integration tests: I manually separate out the vagrant box's cpu 
> and memory between 'aurora-test' role and '*' and verified that jobs can 
> still be launched (I can post the vagrant change in another follow upon 
> request).
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

2016-01-14 Thread Bill Farner 


> On Jan. 13, 2016, 5:06 p.m., Dmitriy Shirchenko wrote:
> > src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java,
> >  lines 216-217
> > 
> >
> > Is this intentionally commented out?
> 
> Zhitao Li wrote:
> Yes this is intentially removed. Old behavior of changing this `config` 
> here resulted in `resources` in static final object `OFFER` not sufficient to 
> be allocated, and I don't see any point to use a different executor for this 
> test case, so I've deleted this code.
> 
> If any reviewer thinks there is a reason to test this with a different 
> executor, I can reconstruct an `Offer` object with correct resource and add 
> this back.
> 
> Bill Farner wrote:
> This is slightly suspicious...are you sure resource accounting wasn't 
> impacted in some way?  I would expect old test cases to logically work with 
> this feature.
> 
> Zhitao Li wrote:
> I recalled why I commented this out in the first pass now (this was a bit 
> tricky and I mistakenly thought it's logical bug while it's due to floating 
> point precision).
> 
> In TaskExecutors.java, `SOME_OVERHEAD_EXECUTOR` uses `0.01` cpus:
> ```
>   public static final ExecutorSettings SOME_OVERHEAD_EXECUTOR =
>   TestExecutorSettings.thermosOnlyWithOverhead(
>   new ResourceSlot(0.01, Amount.of(256L, Data.MB), Amount.of(0L, 
> Data.MB), 0));
> ```
> Unfortunately, `5.01 - 5 - 0.01` **is not zero** in floating point math 
> of Java, so the test keeps throwing `InsufficientResourcesException` even if 
> I constructed an `Offer` whose resource is correctly equivalent to 
> `ResourceSlot.from(TASK_CONFIG).add(TaskExecutors.SOME_OVERHEAD_EXECUTOR.getExecutorOverhead())`.
> 
> **My proposal is to change all comparison with zero in `AcceptedOffer` 
> class (3 places in current patch) to compare with an `EPSILON` whose value is 
> `1e-6` or something even smaller.** I don't think there is any reasonable 
> usage of resource unit smaller than this granularity, and the floating point 
> math in the class would be more robust. We can document this behavior 
> somewhere if it sounds interesting.
> 
> The default resource usage for `THERMOS_EXECUTOR` is 0.25 so I don't 
> encounter this elsewhere.
> 
> Some alternatives which I liked less:
> 1. Change `TaskExecutors.SOME_OVERHEAD_EXECUTOR`'s cpus to a value which 
> could be precisely represented by floating point (0.125 or something similar);
> 2. Manually add a bit more cpu resource to the `Offer` object to trick 
> the test to pass.
> 
> If the above proposed solution sounds fine, I can send an update to the 
> patch.

I'm in favor of using an EPSILON value.  Please be sure to extract a function 
to handle the equivalence check.

It's too bad mesos doesn't use fixed point for scalar resources to avoid these 
issues.  Looks like there's been recent momentum to make that change: 
https://issues.apache.org/jira/browse/MESOS-3997

They recently addressed an instance of this issue with this change:
https://github.com/apache/mesos/commit/5b5dd566aea71f654c51b2706a958ca1b5cb07a3

```
CHECK_NEAR(result.cpus().get(), cpus().get(), MIN_CPUS);
```

For reference, `MIN_CPUS` is 0.01.


- Bill


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


On Jan. 13, 2016, 5:39 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 13, 2016, 5:39 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
> Farner.
> 
> 
> Bugs: AURORA-1109
> https://issues.apache.org/jira/browse/AURORA-1109
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This review is a prototype for introducing multiple role support in Aurora.
> This creates a new class OfferAllocation, which allcoates resources to 
> resources field in TaskInfo and ExecutorInfo from an offer.
> 
> Current implementation prefers reserved resources over shared resources ('*' 
> role) if both are present
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/ResourceSlot.java 
> 7c3d681c216b78eeecebbe950186e5a79c6fe982 
>   src/main/java/org/apache/aurora/scheduler/Resources.java 
> db422a959ee7b982c2a44323de41ad75d1a4