drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Chris Westin" <chriswesti...@gmail.com>
Subject Re: Review Request 32116: DRILL-2219 - concurrent modification exception risk in TopLevelAllocator
Date Mon, 16 Mar 2015 18:22:52 GMT

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



exec/java-exec/src/main/java/org/apache/drill/exec/memory/TopLevelAllocator.java
<https://reviews.apache.org/r/32116/#comment124165>

    Did this make the problem go away? I'm just a little worried about it because neither
MapMaker nor ConcurrentMap make the promise re ConcurrentModificationException that is made
explicitly by ConcurrentHashMap: http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ConcurrentHashMap.html
. It might be safer for us to define our own
    
    public class IdentityKey<T> {  // in drill-common?
      private final T key;
    
      public IdentityKey(T key) {
        Preconditions.checkNotNull(key);
        this.key = key;
      }
    
      @Override
      public boolean equals(Object other) {
        Preconditions.checkNotNull(other);
    
        if (this == other) {
          return true;
        }
    
        if (!(other instanceof IdentityKey)) {
          return false;
        }
        final IdentityKey<?> ik = (IdentityKey<?>) other;
        if (key == ik.key) {
          return true;
        }
    
        return false;
      }
    
      @Override
      public int hashCode() {
        return Objects.hash(key);
      }
    }
    
    Using such a thing wouldn't be any worse than what the weakKeys() option says it is doing,
which is to wrap every key in a weak reference, which will have the same memory characteristics
as using this.


- Chris Westin


On March 16, 2015, 10:50 a.m., Jason Altekruse wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32116/
> -----------------------------------------------------------
> 
> (Updated March 16, 2015, 10:50 a.m.)
> 
> 
> Review request for drill and Chris Westin.
> 
> 
> Bugs: DRILL-2219
>     https://issues.apache.org/jira/browse/DRILL-2219
> 
> 
> Repository: drill-git
> 
> 
> Description
> -------
> 
> We iterate over a list of child allocators in the close method, when close is being called
we are not guarenteed to be free of fragments that can still request a new child allocator
and modify the list (in this case a map because it stores a stack trace list with it).
> 
> 
> Diffs
> -----
> 
>   exec/java-exec/src/main/java/org/apache/drill/exec/memory/TopLevelAllocator.java 67e1fdb

> 
> Diff: https://reviews.apache.org/r/32116/diff/
> 
> 
> Testing
> -------
> 
> Re-running tests on the patch rebased on master, will post results soon.
> 
> 
> Thanks,
> 
> Jason Altekruse
> 
>


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