----------------------------------------------------------- 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) <https://reviews.apache.org/r/39670/#comment162555> 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 > >
