aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zameer Manji" <zma...@apache.org>
Subject Re: Review Request 39670: Create immutable copy of offers for PendingTaskProcessor.
Date Tue, 27 Oct 2015 23:57:31 GMT


> 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

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.


- Zameer


-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message