> On March 16, 2015, 6:22 p.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/memory/TopLevelAllocator.java, > > line 72 > > <https://reviews.apache.org/r/32116/diff/1/?file=896097#file896097line72> > > > > 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. > > Jason Altekruse wrote: > The reason I chose this approach is that this documentation actually > specified "new MapMaker().makeMap() returns a valid concurrent map that > behaves exactly like a ConcurrentHashMap." > > The next line about how weakKeys() impacts the equality check for the map > didn't make me think using it would invalidate the statement about the > behavior similarity to ConcurrentHashMap. > > > http://google-collections.googlecode.com/svn/trunk/javadoc/com/google/common/collect/MapMaker.html
I actually had not tested this as it only happened in a weird case wher I was debugging a query which messed up the timing. The TopLevelAllocator is pretty well separated from the rest of Drill, so I'll go ahead and write an actual unit test to re-produce the issue and confirm that this change fixes it. - Jason ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32116/#review76600 ----------------------------------------------------------- On March 16, 2015, 5:50 p.m., Jason Altekruse wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32116/ > ----------------------------------------------------------- > > (Updated March 16, 2015, 5:50 p.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 > >
