You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by Jason Altekruse <al...@gmail.com> on 2015/03/16 18:50:35 UTC
Review Request 32116: DRILL-2219 - concurrent modification exception
risk in TopLevelAllocator
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32116/
-----------------------------------------------------------
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
Re: Review Request 32116: DRILL-2219 - concurrent modification
exception risk in TopLevelAllocator
Posted by Chris Westin <ch...@gmail.com>.
> 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
>
>
Re: Review Request 32116: DRILL-2219 - concurrent modification
exception risk in TopLevelAllocator
Posted by Jason Altekruse <al...@gmail.com>.
> 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
>
>
Re: Review Request 32116: DRILL-2219 - concurrent modification
exception risk in TopLevelAllocator
Posted by Jason Altekruse <al...@gmail.com>.
> 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.
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
-----------------------------------------------------------
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
>
>
Re: Review Request 32116: DRILL-2219 - concurrent modification
exception risk in TopLevelAllocator
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 32116: DRILL-2219 - concurrent modification
exception risk in TopLevelAllocator
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32116/#review77939
-----------------------------------------------------------
Ship it!
Ship It!
- 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
>
>