-
Notifications
You must be signed in to change notification settings - Fork 29.1k
[SPARK-43309][SPARK-38461][CORE] Extend INTERNAL_ERROR with categories and add error class INTERNAL_ERROR_BROADCAST #40978
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
… INTERNAL_ERROR_BROADCAST
|
The test failure in GitHub Actions "track allocated resources by taskId" seems to be unrelated to this change. |
|
@cloud-fan, @srielau, could you take a look at this? |
| intercept[SparkException] { broadcast.value } | ||
| intercept[SparkException] { broadcast.unpersist(blocking = true) } | ||
| intercept[SparkException] { broadcast.destroy(blocking = true) } | ||
| val e1 = intercept[SparkException] { broadcast.value } |
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.
can we use the checkError util for testing?
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.
nvm, we don't need to check error message here.
|
thanks, merging to master! |
MaxGekk
left a comment
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.
Could you explain, please, why didn't you introduce sub-classes? Like INTERNAL_ERROR.BROADCAST
|
@MaxGekk sub error class can't have dedicated sql state. |
ok, but |
|
Another advantage is that having |
…s and add error class INTERNAL_ERROR_BROADCAST ### What changes were proposed in this pull request? This change is to extend INTERNAL_ERROR with categories, add INTERNAL_ERROR_BROADCAST as an example of it, and change exceptions created in the broadcast package to use that error class. ### Why are the changes needed? This is to better differentiate internal errors from different categories / areas / modules. ### Does this PR introduce _any_ user-facing change? Yes. The exceptions created in the broadcast package will change to be of error class INTERNAL_ERROR_BROADCAST. ### How was this patch tested? Updated unit tests. Closes apache#40978 from bozhang2820/spark-38461. Authored-by: Bo Zhang <bo.zhang@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>
What changes were proposed in this pull request?
This change is to extend INTERNAL_ERROR with categories, add INTERNAL_ERROR_BROADCAST as an example of it, and change exceptions created in the broadcast package to use that error class.
Why are the changes needed?
This is to better differentiate internal errors from different categories / areas / modules.
Does this PR introduce any user-facing change?
Yes. The exceptions created in the broadcast package will change to be of error class INTERNAL_ERROR_BROADCAST.
How was this patch tested?
Updated unit tests.