Re: Review Request 39670: Modify ClusterStateImpl to be thread safe.

2015-10-29 Thread Bill Farner 

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

Ship it!


Ship It!

- Bill Farner


On Oct. 29, 2015, 1:44 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39670/
> ---
> 
> (Updated Oct. 29, 2015, 1:44 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Bugs: AURORA-1510
> https://issues.apache.org/jira/browse/AURORA-1510
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> ClusterStateImpl exposes a synchronized multimap which does not have a 
> thread-safe implementation of `keySet`. This patch revises the `ClusterState` 
> interface to offer more precise operations and modifies `ClusterStateImpl` to 
> makes all of these operations thread safe.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/preemptor/ClusterStateImpl.java 
> 42e2ca49b3c5de997cb5363e3518512d0f5a725e 
>   
> src/test/java/org/apache/aurora/scheduler/preemptor/ClusterStateImplTest.java 
> a1ac922d471013779710e02c0c9ca9f84b506807 
> 
> Diff: https://reviews.apache.org/r/39670/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> 
> If the approach is good, I can update this with benchmark information.
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 39670: Modify ClusterStateImpl to be thread safe.

2015-10-29 Thread Zameer Manji 

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

(Updated Oct. 29, 2015, 1:44 p.m.)


Review request for Aurora, Maxim Khutornenko and Bill Farner.


Changes
---

Bill's feedback.


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


Repository: aurora


Description
---

ClusterStateImpl exposes a synchronized multimap which does not have a 
thread-safe implementation of `keySet`. This patch revises the `ClusterState` 
interface to offer more precise operations and modifies `ClusterStateImpl` to 
makes all of these operations thread safe.


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/preemptor/ClusterStateImpl.java 
42e2ca49b3c5de997cb5363e3518512d0f5a725e 
  src/test/java/org/apache/aurora/scheduler/preemptor/ClusterStateImplTest.java 
a1ac922d471013779710e02c0c9ca9f84b506807 

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


Testing
---

./gradlew build -Pq

If the approach is good, I can update this with benchmark information.


Thanks,

Zameer Manji

Re: Review Request 39670: Modify ClusterStateImpl to be thread safe.

2015-10-29 Thread Zameer Manji 


> On Oct. 28, 2015, 8:02 a.m., Bill Farner wrote:
> > It's not clear to me is why you didn't decide to return an immutable 
> > multimap copy and stay behind the previous interface.  (Based on the 
> > typical characteristics of the map, and the way it's used here i don't 
> > imagine a big perf difference from this patch.)  Did you consider that?

I did consider this approach before, but I could not get the tests to pass. I 
later determined that it was due to the nature of Multimap#equals. I have 
updated the implementation to reflect this.


- Zameer


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


On Oct. 27, 2015, 9:17 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39670/
> ---
> 
> (Updated Oct. 27, 2015, 9:17 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Bugs: AURORA-1510
> https://issues.apache.org/jira/browse/AURORA-1510
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> ClusterStateImpl exposes a synchronized multimap which does not have a 
> thread-safe implementation of `keySet`. This patch revises the `ClusterState` 
> interface to offer more precise operations and modifies `ClusterStateImpl` to 
> makes all of these operations thread safe.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/preemptor/ClusterState.java 
> ce3bc7e6da3f86625c690e26c28ccef67ed9021a 
>   src/main/java/org/apache/aurora/scheduler/preemptor/ClusterStateImpl.java 
> 42e2ca49b3c5de997cb5363e3518512d0f5a725e 
>   
> src/main/java/org/apache/aurora/scheduler/preemptor/PendingTaskProcessor.java 
> 506176769e172b7e9f4ba05c486fe6ab550fb5c3 
>   
> src/test/java/org/apache/aurora/scheduler/preemptor/PendingTaskProcessorTest.java
>  49002d03b190b8642b6251f4d1fbd6663ee6becc 
> 
> Diff: https://reviews.apache.org/r/39670/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> 
> If the approach is good, I can update this with benchmark information.
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 39670: Modify ClusterStateImpl to be thread safe.

2015-10-28 Thread Bill Farner 

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

Ship it!


It's not clear to me is why you didn't decide to return an immutable multimap 
copy and stay behind the previous interface.  (Based on the typical 
characteristics of the map, and the way it's used here i don't imagine a big 
perf difference from this patch.)  Did you consider that?


src/main/java/org/apache/aurora/scheduler/preemptor/ClusterStateImpl.java (line 
57)


This is redundant with what `Maps.synchronizedMultimap` is already doing.  
The others are legitimate though.

That said, i suggest you remove this and instead have the caller invoke 
`isEmpty()` on the result of `getSlavesWithActiveTasks()`.


- Bill Farner


On Oct. 27, 2015, 9:17 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39670/
> ---
> 
> (Updated Oct. 27, 2015, 9:17 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Bugs: AURORA-1510
> https://issues.apache.org/jira/browse/AURORA-1510
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> ClusterStateImpl exposes a synchronized multimap which does not have a 
> thread-safe implementation of `keySet`. This patch revises the `ClusterState` 
> interface to offer more precise operations and modifies `ClusterStateImpl` to 
> makes all of these operations thread safe.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/preemptor/ClusterState.java 
> ce3bc7e6da3f86625c690e26c28ccef67ed9021a 
>   src/main/java/org/apache/aurora/scheduler/preemptor/ClusterStateImpl.java 
> 42e2ca49b3c5de997cb5363e3518512d0f5a725e 
>   
> src/main/java/org/apache/aurora/scheduler/preemptor/PendingTaskProcessor.java 
> 506176769e172b7e9f4ba05c486fe6ab550fb5c3 
>   
> src/test/java/org/apache/aurora/scheduler/preemptor/PendingTaskProcessorTest.java
>  49002d03b190b8642b6251f4d1fbd6663ee6becc 
> 
> Diff: https://reviews.apache.org/r/39670/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> 
> If the approach is good, I can update this with benchmark information.
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 39670: Modify ClusterStateImpl to be thread safe.

2015-10-27 Thread Zameer Manji 

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

(Updated Oct. 27, 2015, 9:12 p.m.)


Review request for Aurora, Maxim Khutornenko and Bill Farner.


Changes
---

Revise implementation.


Summary (updated)
-

Modify ClusterStateImpl to be thread safe.


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


Repository: aurora


Description (updated)
---

ClusterStateImpl exposts a synchronized multimap which does not have a 
thread-safe implementation of `keySet`. This patch revises the `ClusterState` 
interface to offer more precise operations and modifies `ClusterStateImpl` to 
makes all of these operations thread safe.


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/preemptor/ClusterState.java 
ce3bc7e6da3f86625c690e26c28ccef67ed9021a 
  src/main/java/org/apache/aurora/scheduler/preemptor/ClusterStateImpl.java 
42e2ca49b3c5de997cb5363e3518512d0f5a725e 
  src/main/java/org/apache/aurora/scheduler/preemptor/PendingTaskProcessor.java 
506176769e172b7e9f4ba05c486fe6ab550fb5c3 
  
src/test/java/org/apache/aurora/scheduler/preemptor/PendingTaskProcessorTest.java
 49002d03b190b8642b6251f4d1fbd6663ee6becc 

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


Testing (updated)
---

./gradlew build -Pq

If the approach is good, I can update this with benchmark information.


Thanks,

Zameer Manji