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
