Re: Review Request 20066: Remove duplicate task throttling behavior from TaskGroups, fix a few race conditions.

2014-04-07 Thread Bill Farner

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

(Updated April 7, 2014, 3:07 p.m.)


Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.


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


Repository: aurora


Description
---

TaskGroups suffered from (at least) two race conditions:

- When a group is added, the call to TaskGroup#push() would race (and normally 
beat) the async Runnable execution.  However, if the latter won, it would 
result in skipping further evaluations of the group forever.

- A task ID was removed from the TaskGroup while being evaluated.  If the task 
was deleted during this time, the task would be added back to the TaskGroup and 
re-evaluated later.  This case was benign since the subsequent evaluation would 
discover that the task does not exist.

Test cases were added to TaskGroupsTest to reproduce these scenarios.

In addition to these problems, TaskGroups was still performing throttling of 
flapping tasks, despite TaskThrottler being introduced for that purpose.  
Removing that extra code simplifies things quite a bit.


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/async/TaskGroup.java 
ece7e3a46075edfa881c8750c491d35fdb2b98a8 
  src/main/java/org/apache/aurora/scheduler/async/TaskGroups.java 
6c95c6f7695cb8126105818528900cb09887ad3e 
  src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 
263235ce7c279f98920d7681f6162458be40593c 
  src/test/java/org/apache/aurora/scheduler/async/TaskGroupsTest.java 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 
65e00f70cc860b2d225335e4487add2a903e72bb 
  src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 
ce03abc5248dbc95306c759a04828830e3057b81 

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


Testing
---

./gradlew build


Thanks,

Bill Farner



Re: Review Request 20066: Remove duplicate task throttling behavior from TaskGroups, fix a few race conditions.

2014-04-07 Thread Kevin Sweeney

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



src/main/java/org/apache/aurora/scheduler/async/TaskGroup.java
https://reviews.apache.org/r/20066/#comment72351

What does synchronized add here? As far as I can tell this value is 
immutable.


- Kevin Sweeney


On April 7, 2014, 8:07 a.m., Bill Farner wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/20066/
 ---
 
 (Updated April 7, 2014, 8:07 a.m.)
 
 
 Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
 
 
 Bugs: AURORA-302
 https://issues.apache.org/jira/browse/AURORA-302
 
 
 Repository: aurora
 
 
 Description
 ---
 
 TaskGroups suffered from (at least) two race conditions:
 
 - When a group is added, the call to TaskGroup#push() would race (and 
 normally beat) the async Runnable execution.  However, if the latter won, it 
 would result in skipping further evaluations of the group forever.
 
 - A task ID was removed from the TaskGroup while being evaluated.  If the 
 task was deleted during this time, the task would be added back to the 
 TaskGroup and re-evaluated later.  This case was benign since the subsequent 
 evaluation would discover that the task does not exist.
 
 Test cases were added to TaskGroupsTest to reproduce these scenarios.
 
 In addition to these problems, TaskGroups was still performing throttling of 
 flapping tasks, despite TaskThrottler being introduced for that purpose.  
 Removing that extra code simplifies things quite a bit.
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/async/TaskGroup.java 
 ece7e3a46075edfa881c8750c491d35fdb2b98a8 
   src/main/java/org/apache/aurora/scheduler/async/TaskGroups.java 
 6c95c6f7695cb8126105818528900cb09887ad3e 
   src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 
 263235ce7c279f98920d7681f6162458be40593c 
   src/test/java/org/apache/aurora/scheduler/async/TaskGroupsTest.java 
 PRE-CREATION 
   src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 
 65e00f70cc860b2d225335e4487add2a903e72bb 
   src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 
 ce03abc5248dbc95306c759a04828830e3057b81 
 
 Diff: https://reviews.apache.org/r/20066/diff/
 
 
 Testing
 ---
 
 ./gradlew build
 
 
 Thanks,
 
 Bill Farner
 




Re: Review Request 20066: Remove duplicate task throttling behavior from TaskGroups, fix a few race conditions.

2014-04-07 Thread Kevin Sweeney

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

Ship it!


Ship It!

- Kevin Sweeney


On April 7, 2014, 8:07 a.m., Bill Farner wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/20066/
 ---
 
 (Updated April 7, 2014, 8:07 a.m.)
 
 
 Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
 
 
 Bugs: AURORA-302
 https://issues.apache.org/jira/browse/AURORA-302
 
 
 Repository: aurora
 
 
 Description
 ---
 
 TaskGroups suffered from (at least) two race conditions:
 
 - When a group is added, the call to TaskGroup#push() would race (and 
 normally beat) the async Runnable execution.  However, if the latter won, it 
 would result in skipping further evaluations of the group forever.
 
 - A task ID was removed from the TaskGroup while being evaluated.  If the 
 task was deleted during this time, the task would be added back to the 
 TaskGroup and re-evaluated later.  This case was benign since the subsequent 
 evaluation would discover that the task does not exist.
 
 Test cases were added to TaskGroupsTest to reproduce these scenarios.
 
 In addition to these problems, TaskGroups was still performing throttling of 
 flapping tasks, despite TaskThrottler being introduced for that purpose.  
 Removing that extra code simplifies things quite a bit.
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/async/TaskGroup.java 
 ece7e3a46075edfa881c8750c491d35fdb2b98a8 
   src/main/java/org/apache/aurora/scheduler/async/TaskGroups.java 
 6c95c6f7695cb8126105818528900cb09887ad3e 
   src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 
 263235ce7c279f98920d7681f6162458be40593c 
   src/test/java/org/apache/aurora/scheduler/async/TaskGroupsTest.java 
 PRE-CREATION 
   src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 
 65e00f70cc860b2d225335e4487add2a903e72bb 
   src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 
 ce03abc5248dbc95306c759a04828830e3057b81 
 
 Diff: https://reviews.apache.org/r/20066/diff/
 
 
 Testing
 ---
 
 ./gradlew build
 
 
 Thanks,
 
 Bill Farner
 




Re: Review Request 20066: Remove duplicate task throttling behavior from TaskGroups, fix a few race conditions.

2014-04-07 Thread Bill Farner


 On April 7, 2014, 11:07 p.m., Kevin Sweeney wrote:
  src/main/java/org/apache/aurora/scheduler/async/TaskGroup.java, line 62
  https://reviews.apache.org/r/20066/diff/3/?file=550680#file550680line62
 
  What does synchronized add here? As far as I can tell this value is 
  immutable.

You're absolutely right.
This is a pattern i've converged towards when using intrinsic locks — 
synchronize all public methods (or otherwise externally-accessible code paths). 
 The motivation is to simplify visual scans of thread [un]safety and (to a 
lesser degree) future-proof from new functionality and copy/paste.


- Bill


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


On April 7, 2014, 3:07 p.m., Bill Farner wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/20066/
 ---
 
 (Updated April 7, 2014, 3:07 p.m.)
 
 
 Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
 
 
 Bugs: AURORA-302
 https://issues.apache.org/jira/browse/AURORA-302
 
 
 Repository: aurora
 
 
 Description
 ---
 
 TaskGroups suffered from (at least) two race conditions:
 
 - When a group is added, the call to TaskGroup#push() would race (and 
 normally beat) the async Runnable execution.  However, if the latter won, it 
 would result in skipping further evaluations of the group forever.
 
 - A task ID was removed from the TaskGroup while being evaluated.  If the 
 task was deleted during this time, the task would be added back to the 
 TaskGroup and re-evaluated later.  This case was benign since the subsequent 
 evaluation would discover that the task does not exist.
 
 Test cases were added to TaskGroupsTest to reproduce these scenarios.
 
 In addition to these problems, TaskGroups was still performing throttling of 
 flapping tasks, despite TaskThrottler being introduced for that purpose.  
 Removing that extra code simplifies things quite a bit.
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/async/TaskGroup.java 
 ece7e3a46075edfa881c8750c491d35fdb2b98a8 
   src/main/java/org/apache/aurora/scheduler/async/TaskGroups.java 
 6c95c6f7695cb8126105818528900cb09887ad3e 
   src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 
 263235ce7c279f98920d7681f6162458be40593c 
   src/test/java/org/apache/aurora/scheduler/async/TaskGroupsTest.java 
 PRE-CREATION 
   src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 
 65e00f70cc860b2d225335e4487add2a903e72bb 
   src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 
 ce03abc5248dbc95306c759a04828830e3057b81 
 
 Diff: https://reviews.apache.org/r/20066/diff/
 
 
 Testing
 ---
 
 ./gradlew build
 
 
 Thanks,
 
 Bill Farner
 




Re: Review Request 20066: Remove duplicate task throttling behavior from TaskGroups, fix a few race conditions.

2014-04-06 Thread Bill Farner

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

(Updated April 6, 2014, 7:29 p.m.)


Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.


Summary (updated)
-

Remove duplicate task throttling behavior from TaskGroups, fix a few race 
conditions.


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


Repository: aurora


Description
---

TaskGroups suffered from (at least) two race conditions:

- When a group is added, the call to TaskGroup#push() would race (and normally 
beat) the async Runnable execution.  However, if the latter won, it would 
result in skipping further evaluations of the group forever.

- A task ID was removed from the TaskGroup while being evaluated.  If the task 
was deleted during this time, the task would be added back to the TaskGroup and 
re-evaluated later.  This case was benign since the subsequent evaluation would 
discover that the task does not exist.

Test cases were added to TaskGroupsTest to reproduce these scenarios.

In addition to these problems, TaskGroups was still performing throttling of 
flapping tasks, despite TaskThrottler being introduced for that purpose.  
Removing that extra code simplifies things quite a bit.


Diffs
-

  src/main/java/org/apache/aurora/scheduler/async/TaskGroup.java 
ece7e3a46075edfa881c8750c491d35fdb2b98a8 
  src/main/java/org/apache/aurora/scheduler/async/TaskGroups.java 
6c95c6f7695cb8126105818528900cb09887ad3e 
  src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 
263235ce7c279f98920d7681f6162458be40593c 
  src/test/java/org/apache/aurora/scheduler/async/TaskGroupsTest.java 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 
65e00f70cc860b2d225335e4487add2a903e72bb 
  src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 
ce03abc5248dbc95306c759a04828830e3057b81 

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


Testing
---

./gradlew build


Thanks,

Bill Farner



Re: Review Request 20066: Remove duplicate task throttling behavior from TaskGroups, fix a few race conditions.

2014-04-06 Thread Bill Farner

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

(Updated April 6, 2014, 7:40 p.m.)


Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.


Changes
---

Remove old TODO.


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


Repository: aurora


Description
---

TaskGroups suffered from (at least) two race conditions:

- When a group is added, the call to TaskGroup#push() would race (and normally 
beat) the async Runnable execution.  However, if the latter won, it would 
result in skipping further evaluations of the group forever.

- A task ID was removed from the TaskGroup while being evaluated.  If the task 
was deleted during this time, the task would be added back to the TaskGroup and 
re-evaluated later.  This case was benign since the subsequent evaluation would 
discover that the task does not exist.

Test cases were added to TaskGroupsTest to reproduce these scenarios.

In addition to these problems, TaskGroups was still performing throttling of 
flapping tasks, despite TaskThrottler being introduced for that purpose.  
Removing that extra code simplifies things quite a bit.


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/async/TaskGroup.java 
ece7e3a46075edfa881c8750c491d35fdb2b98a8 
  src/main/java/org/apache/aurora/scheduler/async/TaskGroups.java 
6c95c6f7695cb8126105818528900cb09887ad3e 
  src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 
263235ce7c279f98920d7681f6162458be40593c 
  src/test/java/org/apache/aurora/scheduler/async/TaskGroupsTest.java 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 
65e00f70cc860b2d225335e4487add2a903e72bb 
  src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 
ce03abc5248dbc95306c759a04828830e3057b81 

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


Testing
---

./gradlew build


Thanks,

Bill Farner



Re: Review Request 20066: Remove duplicate task throttling behavior from TaskGroups, fix a few race conditions.

2014-04-06 Thread Maxim Khutornenko

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

Ship it!



src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java
https://reviews.apache.org/r/20066/#comment72219

This is no longer relevant.


- Maxim Khutornenko


On April 6, 2014, 7:40 p.m., Bill Farner wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/20066/
 ---
 
 (Updated April 6, 2014, 7:40 p.m.)
 
 
 Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
 
 
 Bugs: AURORA-302
 https://issues.apache.org/jira/browse/AURORA-302
 
 
 Repository: aurora
 
 
 Description
 ---
 
 TaskGroups suffered from (at least) two race conditions:
 
 - When a group is added, the call to TaskGroup#push() would race (and 
 normally beat) the async Runnable execution.  However, if the latter won, it 
 would result in skipping further evaluations of the group forever.
 
 - A task ID was removed from the TaskGroup while being evaluated.  If the 
 task was deleted during this time, the task would be added back to the 
 TaskGroup and re-evaluated later.  This case was benign since the subsequent 
 evaluation would discover that the task does not exist.
 
 Test cases were added to TaskGroupsTest to reproduce these scenarios.
 
 In addition to these problems, TaskGroups was still performing throttling of 
 flapping tasks, despite TaskThrottler being introduced for that purpose.  
 Removing that extra code simplifies things quite a bit.
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/async/TaskGroup.java 
 ece7e3a46075edfa881c8750c491d35fdb2b98a8 
   src/main/java/org/apache/aurora/scheduler/async/TaskGroups.java 
 6c95c6f7695cb8126105818528900cb09887ad3e 
   src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 
 263235ce7c279f98920d7681f6162458be40593c 
   src/test/java/org/apache/aurora/scheduler/async/TaskGroupsTest.java 
 PRE-CREATION 
   src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 
 65e00f70cc860b2d225335e4487add2a903e72bb 
   src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 
 ce03abc5248dbc95306c759a04828830e3057b81 
 
 Diff: https://reviews.apache.org/r/20066/diff/
 
 
 Testing
 ---
 
 ./gradlew build
 
 
 Thanks,
 
 Bill Farner