HIVE-15221: Improvement for MapJoin checkMemoryStatus, adding gc befor… #5562
Conversation
|
BsoBird
changed the title
|
@zhangbutao |
| @@ -96,7 +96,12 @@ public void checkMemoryStatus(long tableContainerSize, long numRows) | |||
| + tableContainerSize + "\tMemory usage:\t" + usedMemory + "\tpercentage:\t" + percentageNumberFormat.format(percentage); | |||
| console.printInfo(msg); | |||
| if(percentage > maxMemoryUsage) { | |||
| throw new MapJoinMemoryExhaustionError(msg); | |||
| System.gc(); | |||
There was a problem hiding this comment.
I am not sure how to test this code. Do you have some ut or local test to show that this code work well?
btw, I think adding a configuration will be good here & Keep the conf disabled.
There was a problem hiding this comment.
Is this safe or desirable with LLAP?
There was a problem hiding this comment.
@zhangbutao @okumin
As a whole, this patch doesn't change any execution logic, it just rechecks the memory space after trying to call system.gc() once in case of out-of-memory.
There was a problem hiding this comment.
I wondered if we can use System.gc, a JVM global method, for a task-local problem. I basically don't know why we implemented MapJoinMemoryExhaustionHandler. I might be saying something strange
There was a problem hiding this comment.
I've used this patch in our production environment, and the OOM and MapJoinMemoryExhaustionError related issues have really been reduced. However, I do agree with you to some extent that the current implementation is a bit problematic.
There was a problem hiding this comment.
First, I might not have sufficient knowledge to give this +1. I've never faced this error.
Reading the source code, I found that only Hive on MapReduce uses MemoryExhaustionChecker. So, not only LLAP but also Tez are not involved with this error. My original concern, i.e., the entire LLAP daemon could get degraded, is gone.
But I am still wondering why Hive on MapReduce needs MemoryExhaustionChecker. We may remove the class and let MapReduce crush with OOM when creating a giant hash table.
There was a problem hiding this comment.
@okumin Hello. Although I haven't carefully reviewed this part of the code yet, using the TEZ engine will still trigger the MapJoinMemoryExhaustionError.
…e throwing Exception
https://issues.apache.org/jira/browse/HIVE-15221
What changes were proposed in this pull request?
Why are the changes needed?
Does this PR introduce any user-facing change?
Is the change a dependency upgrade?
How was this patch tested?