Re: Review Request 28607: Add a caching ClusterState implementation.
On Dec. 3, 2014, 2:06 a.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterState.java, line 56 https://reviews.apache.org/r/28607/diff/2/?file=780608#file780608line56 This can be further simplified (unless you are concerned about the heap churn): ```java victims.remove(slaveId, PreemptionVictim.fromTask(stateChange.getTask().getAssignedTask()); ``` Good suggestion, this also removed some surrounding code. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28607/#review63637 --- On Dec. 3, 2014, 1:39 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28607/ --- (Updated Dec. 3, 2014, 1:39 a.m.) Review request for Aurora, Maxim Khutornenko and Zameer Manji. Bugs: AURORA-121 https://issues.apache.org/jira/browse/AURORA-121 Repository: aurora Description --- Add a caching ClusterState implementation. Diffs - src/main/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterState.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/async/preemptor/ClusterState.java 3524dc595e7b61a531912843f90b01a87bc57cc4 src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java 489c0bfa22aae14fbd3fd2ee9e5bd32b7c3aafa5 src/test/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterStateTest.java PRE-CREATION Diff: https://reviews.apache.org/r/28607/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 28607: Add a caching ClusterState implementation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28607/ --- (Updated Dec. 3, 2014, 6:23 p.m.) Review request for Aurora, Maxim Khutornenko and Zameer Manji. Bugs: AURORA-121 https://issues.apache.org/jira/browse/AURORA-121 Repository: aurora Description --- Add a caching ClusterState implementation. Diffs (updated) - src/main/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterState.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/async/preemptor/ClusterState.java 3524dc595e7b61a531912843f90b01a87bc57cc4 src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java 489c0bfa22aae14fbd3fd2ee9e5bd32b7c3aafa5 src/test/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterStateTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptionVictimTest.java PRE-CREATION Diff: https://reviews.apache.org/r/28607/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 28607: Add a caching ClusterState implementation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28607/#review63700 --- Ship it! Master (8959b49) 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 Dec. 3, 2014, 6:23 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28607/ --- (Updated Dec. 3, 2014, 6:23 p.m.) Review request for Aurora, Maxim Khutornenko and Zameer Manji. Bugs: AURORA-121 https://issues.apache.org/jira/browse/AURORA-121 Repository: aurora Description --- Add a caching ClusterState implementation. Diffs - src/main/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterState.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/async/preemptor/ClusterState.java 3524dc595e7b61a531912843f90b01a87bc57cc4 src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java 489c0bfa22aae14fbd3fd2ee9e5bd32b7c3aafa5 src/test/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterStateTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptionVictimTest.java PRE-CREATION Diff: https://reviews.apache.org/r/28607/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 28607: Add a caching ClusterState implementation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28607/#review63596 --- Ship it! Master (c86ca6d) 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 Dec. 2, 2014, 10:03 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28607/ --- (Updated Dec. 2, 2014, 10:03 p.m.) Review request for Aurora, Maxim Khutornenko and Zameer Manji. Bugs: AURORA-121 https://issues.apache.org/jira/browse/AURORA-121 Repository: aurora Description --- Add a caching ClusterState implementation. Diffs - src/main/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterState.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/async/preemptor/ClusterState.java 3524dc595e7b61a531912843f90b01a87bc57cc4 src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java 489c0bfa22aae14fbd3fd2ee9e5bd32b7c3aafa5 src/test/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterStateTest.java PRE-CREATION Diff: https://reviews.apache.org/r/28607/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 28607: Add a caching ClusterState implementation.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28607/#review63598
---
src/main/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterState.java
https://reviews.apache.org/r/28607/#comment105873
minor nit: you might want to have it outside the synchronized block to
further reduce lock scope.
src/main/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterState.java
https://reviews.apache.org/r/28607/#comment105877
Would it make sense to do it conditionally, i.e.:
```java
if (!Tasks.SLAVE_ASSIGNED_STATES.contains(stateChange.getNewState()) {
Iterables.remove...
} else {
victims.put...
}
```
- Maxim Khutornenko
On Dec. 2, 2014, 10:03 p.m., Bill Farner wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28607/
---
(Updated Dec. 2, 2014, 10:03 p.m.)
Review request for Aurora, Maxim Khutornenko and Zameer Manji.
Bugs: AURORA-121
https://issues.apache.org/jira/browse/AURORA-121
Repository: aurora
Description
---
Add a caching ClusterState implementation.
Diffs
-
src/main/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterState.java
PRE-CREATION
src/main/java/org/apache/aurora/scheduler/async/preemptor/ClusterState.java
3524dc595e7b61a531912843f90b01a87bc57cc4
src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java
489c0bfa22aae14fbd3fd2ee9e5bd32b7c3aafa5
src/test/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterStateTest.java
PRE-CREATION
Diff: https://reviews.apache.org/r/28607/diff/
Testing
---
Thanks,
Bill Farner
Re: Review Request 28607: Add a caching ClusterState implementation.
On Dec. 2, 2014, 11:09 p.m., Maxim Khutornenko wrote:
src/main/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterState.java,
line 52
https://reviews.apache.org/r/28607/diff/1/?file=780416#file780416line52
minor nit: you might want to have it outside the synchronized block to
further reduce lock scope.
I prefer the current approach, since there's less likelihood for a future diff
that operates on the map outside the synchronized block.
On Dec. 2, 2014, 11:09 p.m., Maxim Khutornenko wrote:
src/main/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterState.java,
line 58
https://reviews.apache.org/r/28607/diff/1/?file=780416#file780416line58
Would it make sense to do it conditionally, i.e.:
```java
if (!Tasks.SLAVE_ASSIGNED_STATES.contains(stateChange.getNewState()) {
Iterables.remove...
} else {
victims.put...
}
```
That would introduce duplicates, e.g. PENDING - ASSIGNED - RUNNING would
result in 2 entries for the task.
- Bill
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28607/#review63598
---
On Dec. 2, 2014, 10:03 p.m., Bill Farner wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28607/
---
(Updated Dec. 2, 2014, 10:03 p.m.)
Review request for Aurora, Maxim Khutornenko and Zameer Manji.
Bugs: AURORA-121
https://issues.apache.org/jira/browse/AURORA-121
Repository: aurora
Description
---
Add a caching ClusterState implementation.
Diffs
-
src/main/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterState.java
PRE-CREATION
src/main/java/org/apache/aurora/scheduler/async/preemptor/ClusterState.java
3524dc595e7b61a531912843f90b01a87bc57cc4
src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java
489c0bfa22aae14fbd3fd2ee9e5bd32b7c3aafa5
src/test/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterStateTest.java
PRE-CREATION
Diff: https://reviews.apache.org/r/28607/diff/
Testing
---
Thanks,
Bill Farner
Re: Review Request 28607: Add a caching ClusterState implementation.
On Dec. 2, 2014, 11:09 p.m., Maxim Khutornenko wrote:
src/main/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterState.java,
line 58
https://reviews.apache.org/r/28607/diff/1/?file=780416#file780416line58
Would it make sense to do it conditionally, i.e.:
```java
if (!Tasks.SLAVE_ASSIGNED_STATES.contains(stateChange.getNewState()) {
Iterables.remove...
} else {
victims.put...
}
```
Bill Farner wrote:
That would introduce duplicates, e.g. PENDING - ASSIGNED - RUNNING
would result in 2 entries for the task.
Why not use a HashMultimap to store victims then? I don't see anything in the
PreemptionVictim that would be different between task state transitions.
- Maxim
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28607/#review63598
---
On Dec. 2, 2014, 10:03 p.m., Bill Farner wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28607/
---
(Updated Dec. 2, 2014, 10:03 p.m.)
Review request for Aurora, Maxim Khutornenko and Zameer Manji.
Bugs: AURORA-121
https://issues.apache.org/jira/browse/AURORA-121
Repository: aurora
Description
---
Add a caching ClusterState implementation.
Diffs
-
src/main/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterState.java
PRE-CREATION
src/main/java/org/apache/aurora/scheduler/async/preemptor/ClusterState.java
3524dc595e7b61a531912843f90b01a87bc57cc4
src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java
489c0bfa22aae14fbd3fd2ee9e5bd32b7c3aafa5
src/test/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterStateTest.java
PRE-CREATION
Diff: https://reviews.apache.org/r/28607/diff/
Testing
---
Thanks,
Bill Farner
Re: Review Request 28607: Add a caching ClusterState implementation.
On Dec. 2, 2014, 11:09 p.m., Maxim Khutornenko wrote:
src/main/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterState.java,
line 58
https://reviews.apache.org/r/28607/diff/1/?file=780416#file780416line58
Would it make sense to do it conditionally, i.e.:
```java
if (!Tasks.SLAVE_ASSIGNED_STATES.contains(stateChange.getNewState()) {
Iterables.remove...
} else {
victims.put...
}
```
Bill Farner wrote:
That would introduce duplicates, e.g. PENDING - ASSIGNED - RUNNING
would result in 2 entries for the task.
Maxim Khutornenko wrote:
Why not use a HashMultimap to store victims then? I don't see anything in
the PreemptionVictim that would be different between task state transitions.
Sure, that works for me. Done.
- Bill
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28607/#review63598
---
On Dec. 3, 2014, 1:39 a.m., Bill Farner wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28607/
---
(Updated Dec. 3, 2014, 1:39 a.m.)
Review request for Aurora, Maxim Khutornenko and Zameer Manji.
Bugs: AURORA-121
https://issues.apache.org/jira/browse/AURORA-121
Repository: aurora
Description
---
Add a caching ClusterState implementation.
Diffs
-
src/main/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterState.java
PRE-CREATION
src/main/java/org/apache/aurora/scheduler/async/preemptor/ClusterState.java
3524dc595e7b61a531912843f90b01a87bc57cc4
src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java
489c0bfa22aae14fbd3fd2ee9e5bd32b7c3aafa5
src/test/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterStateTest.java
PRE-CREATION
Diff: https://reviews.apache.org/r/28607/diff/
Testing
---
Thanks,
Bill Farner
Re: Review Request 28607: Add a caching ClusterState implementation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28607/ --- (Updated Dec. 3, 2014, 1:39 a.m.) Review request for Aurora, Maxim Khutornenko and Zameer Manji. Bugs: AURORA-121 https://issues.apache.org/jira/browse/AURORA-121 Repository: aurora Description --- Add a caching ClusterState implementation. Diffs (updated) - src/main/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterState.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/async/preemptor/ClusterState.java 3524dc595e7b61a531912843f90b01a87bc57cc4 src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java 489c0bfa22aae14fbd3fd2ee9e5bd32b7c3aafa5 src/test/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterStateTest.java PRE-CREATION Diff: https://reviews.apache.org/r/28607/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 28607: Add a caching ClusterState implementation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28607/#review63632 --- Ship it! Master (552ae93) 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 Dec. 3, 2014, 1:39 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28607/ --- (Updated Dec. 3, 2014, 1:39 a.m.) Review request for Aurora, Maxim Khutornenko and Zameer Manji. Bugs: AURORA-121 https://issues.apache.org/jira/browse/AURORA-121 Repository: aurora Description --- Add a caching ClusterState implementation. Diffs - src/main/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterState.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/async/preemptor/ClusterState.java 3524dc595e7b61a531912843f90b01a87bc57cc4 src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java 489c0bfa22aae14fbd3fd2ee9e5bd32b7c3aafa5 src/test/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterStateTest.java PRE-CREATION Diff: https://reviews.apache.org/r/28607/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 28607: Add a caching ClusterState implementation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28607/#review63637 --- Ship it! src/main/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterState.java https://reviews.apache.org/r/28607/#comment105910 This can be further simplified (unless you are concerned about the heap churn): ```java victims.remove(slaveId, PreemptionVictim.fromTask(stateChange.getTask().getAssignedTask()); ``` - Maxim Khutornenko On Dec. 3, 2014, 1:39 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28607/ --- (Updated Dec. 3, 2014, 1:39 a.m.) Review request for Aurora, Maxim Khutornenko and Zameer Manji. Bugs: AURORA-121 https://issues.apache.org/jira/browse/AURORA-121 Repository: aurora Description --- Add a caching ClusterState implementation. Diffs - src/main/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterState.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/async/preemptor/ClusterState.java 3524dc595e7b61a531912843f90b01a87bc57cc4 src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java 489c0bfa22aae14fbd3fd2ee9e5bd32b7c3aafa5 src/test/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterStateTest.java PRE-CREATION Diff: https://reviews.apache.org/r/28607/diff/ Testing --- Thanks, Bill Farner
