-
Notifications
You must be signed in to change notification settings - Fork 28.6k
[SPARK-22827][SQL][FOLLOW-UP] Throw SparkOutOfMemoryError
in HashAggregateExec
, too.
#22969
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@@ -787,7 +789,7 @@ case class HashAggregateExec( | |||
| $unsafeRowKeys, ${hashEval.value}); | |||
| if ($unsafeRowBuffer == null) { | |||
| // failed to allocate the first page | |||
| throw new OutOfMemoryError("No enough memory for aggregation"); | |||
| throw new $oomeClassName("No enough memory for aggregation"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @ueshin . Is this the final place? If not, can we have a separate JIRA issue for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think so based on my investigation. I grep-ed with "OutOfMemoryError" and checked the suspicious places.
Test build #98577 has finished for PR 22969 at commit
|
Retest this please. |
Test build #98585 has finished for PR 22969 at commit
|
Merged to master. |
Thank you, @ueshin and @cloud-fan . |
@@ -787,7 +789,7 @@ case class HashAggregateExec( | |||
| $unsafeRowKeys, ${hashEval.value}); | |||
| if ($unsafeRowBuffer == null) { | |||
| // failed to allocate the first page | |||
| throw new OutOfMemoryError("No enough memory for aggregation"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opened a JIRA for banning this by a new lint rule: https://issues.apache.org/jira/browse/SPARK-25986
## What changes were proposed in this pull request? Add scala and java lint check rules to ban the usage of `throw new xxxErrors` and fix up all exists instance followed by #22989 (comment). See more details in #22969. ## How was this patch tested? Local test with lint-scala and lint-java. Closes #22989 from xuanyuanking/SPARK-25986. Authored-by: Yuanjian Li <[email protected]> Signed-off-by: Sean Owen <[email protected]>
…ggregateExec`, too. ## What changes were proposed in this pull request? This is a follow-up pr of apache#20014 which introduced `SparkOutOfMemoryError` to avoid killing the entire executor when an `OutOfMemoryError` is thrown. We should throw `SparkOutOfMemoryError` in `HashAggregateExec`, too. ## How was this patch tested? Existing tests. Closes apache#22969 from ueshin/issues/SPARK-22827/oome. Authored-by: Takuya UESHIN <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>
## What changes were proposed in this pull request? Add scala and java lint check rules to ban the usage of `throw new xxxErrors` and fix up all exists instance followed by apache#22989 (comment). See more details in apache#22969. ## How was this patch tested? Local test with lint-scala and lint-java. Closes apache#22989 from xuanyuanking/SPARK-25986. Authored-by: Yuanjian Li <[email protected]> Signed-off-by: Sean Owen <[email protected]>
What changes were proposed in this pull request?
This is a follow-up pr of #20014 which introduced
SparkOutOfMemoryError
to avoid killing the entire executor when anOutOfMemoryError
is thrown.We should throw
SparkOutOfMemoryError
inHashAggregateExec
, too.How was this patch tested?
Existing tests.