> On Oct. 26, 2015, 5:02 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/preemptor/PendingTaskProcessor.java, > > line 124 > > <https://reviews.apache.org/r/39670/diff/2/?file=1108519#file1108519line124> > > > > My money is on this being the problem. The stack trace in the ticket > > lines up better too. > > Zameer Manji wrote: > Looking at the stack trace, I think I agree but I noticed that > `ClusterStateImpl` is backed by a synchronized multimap. I'm not sure how it > can be the problem. Any thoughts here? > > ```` > private final Multimap<String, PreemptionVictim> victims = > Multimaps.synchronizedMultimap(HashMultimap.create()); > > @Override > public Multimap<String, PreemptionVictim> getSlavesToActiveTasks() { > return Multimaps.unmodifiableMultimap(victims); > } > ```` > > Bill Farner wrote: > A synchronized [multi]map just means that the `Map` methods are > synchronized. The iterators can still require synchronization and throw > `ConcurrentModificationException`. Some background here: > http://docs.oracle.com/javase/6/docs/api/java/util/Collections.html#synchronizedCollection%28java.util.Collection%29 > > Zameer Manji wrote: > The Guava documentation agrees: > ``` > * <p>It is imperative that the user manually synchronize on the > returned > * multimap when accessing any of its collection views: <pre> {@code > * > * Multimap<K, V> multimap = Multimaps.synchronizedMultimap( > * HashMultimap.<K, V>create()); > * ... > * Collection<V> values = multimap.get(key); // Needn't be in > synchronized block > * ... > * synchronized (multimap) { // Synchronizing on multimap, not > values! > * Iterator<V> i = values.iterator(); // Must be in synchronized > block > * while (i.hasNext()) { > * foo(i.next()); > * } > * }}</pre> > * > * <p>Failure to follow this advice may result in non-deterministic > behavior. > ``` > > We call `slavesToActiveTasks.keySet()` which is a view over the > underlying multimap. I'll update this patch to synchronize on the returned > multimap before we iterate over the keySet.
Just take care to keep LoD in mind to make sure you're not making assumptions about the remote class implementation. Synchronizing may also have adverse performance effects. Not saying don't do it - just think about potential ramifications. - Bill ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39670/#review104114 ----------------------------------------------------------- On Oct. 26, 2015, 4:16 p.m., Zameer Manji wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39670/ > ----------------------------------------------------------- > > (Updated Oct. 26, 2015, 4:16 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 > ------- > > The PendingTaskProcessor runs concurrently with MesosSchedulerImpl. > MesosSchedulerImpl adds/removes offfers from OfferManager while > PendingTaskProcessor iterates over the available offers from OfferManager. > Since OfferManager only returns an unmodifiable view of the underlying list > of offers, this causes a `ConcurrentModificationException`. To prevent this > exception, the PendingTaskProcessor takes an immutable copy of the offers > before iterating over the list of available offers. > > > Diffs > ----- > > > src/main/java/org/apache/aurora/scheduler/preemptor/PendingTaskProcessor.java > 506176769e172b7e9f4ba05c486fe6ab550fb5c3 > > Diff: https://reviews.apache.org/r/39670/diff/ > > > Testing > ------- > > ./gradlew build -Pq > > > Thanks, > > Zameer Manji > >
