[SPARK-38461][CORE] Use error classes in org.apache.spark.broadcast#35960
[SPARK-38461][CORE] Use error classes in org.apache.spark.broadcast#35960bozhang2820 wants to merge 1 commit intoapache:masterfrom
Conversation
| numCores: Int, | ||
| mockOutputCommitCoordinator: Option[OutputCommitCoordinator] = None): SparkEnv = { | ||
| mockOutputCommitCoordinator: Option[OutputCommitCoordinator] = None, | ||
| blockManagerOption: Option[BlockManager] = None): SparkEnv = { |
There was a problem hiding this comment.
| blockManagerOption: Option[BlockManager] = None): SparkEnv = { | |
| mockOutputCommitCoordinator: Option[BlockManager] = None): SparkEnv = { |
to follow mockOutputCommitCoordinator above? It is also more clear that this is used as mock one in test only.
mridulm
left a comment
mridulm
left a comment
There was a problem hiding this comment.
Took a quick pass through the PR.
| logError(s"Store broadcast $broadcastId fail, remove all pieces of the broadcast") | ||
| blockManager.removeBroadcast(id, tellMaster = true) | ||
| throw t | ||
| throw SparkCoreErrors.storeBlockError(broadcastId, t) |
There was a problem hiding this comment.
Two issues with this:
a) We are changing the exception being thrown (note: we are catching Throwable).
b) Message for exception thrown in L151 is getting changed in L160.
There was a problem hiding this comment.
Will change back to throw the caught Throwable here.
| new SparkException(errorClass = "STORE_BLOCK_ERROR", | ||
| messageParameters = Array(blockId.toString), cause = cause) | ||
| } | ||
|
|
There was a problem hiding this comment.
Let us remove getBlockError, corruptedRemoteBlockError and getLocalBlockError - they are replacing a single invocation site.
There was a problem hiding this comment.
Not sure what you mean by "replacing a single invocation site". Could you elaborate?
| override private[spark] def createSparkEnv( | ||
| conf: SparkConf, | ||
| isLocal: Boolean, | ||
| listenerBus: LiveListenerBus) = { |
|
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable. |
3 participants
What changes were proposed in this pull request?
This change is to refactor exceptions thrown in org.apache.spark.broadcast to use error class framework.
Why are the changes needed?
This is to follow the error class framework.
Does this PR introduce any user-facing change?
Yes. To aggregate the exceptions, there are minor changes in error messages in the exceptions thrown.
How was this patch tested?
Added unit tests.