> On March 16, 2015, 11:22 a.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 > > Jason Altekruse wrote: > 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.
If the above says it "behaves exactly like a ConcurrentHashMap," (I missed that), then it includes the promise. So just go with it. If we have problems again, we can fall back to what I've suggested. Re the weakKeys() comment, no, it doesn't invalidate the concurrent behavior, I was just commenting on the memory usage characteristics relative to the introduction of a key wrapper. - Chris ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32116/#review76600 ----------------------------------------------------------- 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 > >
