You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "bozhang2820 (via GitHub)" <gi...@apache.org> on 2023/05/11 11:19:54 UTC
[GitHub] [spark] bozhang2820 opened a new pull request, #41140: [SPARK-38469][CORE] Use error class in org.apache.spark.network
bozhang2820 opened a new pull request, #41140:
URL: https://github.com/apache/spark/pull/41140
### What changes were proposed in this pull request?
This PR aims to change exceptions created in package org.apache.spark.netrowk to use error class. This also adds an error class INTERNAL_ERROR_NETWORK and uses that for the internal errors in the package.
### Why are the changes needed?
This is to move exceptions created in package org.apache.spark.network to error class.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Existing tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] bozhang2820 commented on pull request #41140: [SPARK-38469][CORE] Use error class in org.apache.spark.network
Posted by "bozhang2820 (via GitHub)" <gi...@apache.org>.
bozhang2820 commented on PR #41140:
URL: https://github.com/apache/spark/pull/41140#issuecomment-1547714691
CI test `single listener, check trigger events are generated correctly` failed, which should be irrelevant to this change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on pull request #41140: [SPARK-38469][CORE] Use error class in org.apache.spark.network
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #41140:
URL: https://github.com/apache/spark/pull/41140#issuecomment-1546607319
Waiting for CI.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk closed pull request #41140: [SPARK-38469][CORE] Use error class in org.apache.spark.network
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #41140: [SPARK-38469][CORE] Use error class in org.apache.spark.network
URL: https://github.com/apache/spark/pull/41140
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a diff in pull request #41140: [SPARK-38469][CORE] Use error class in org.apache.spark.network
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #41140:
URL: https://github.com/apache/spark/pull/41140#discussion_r1191221294
##########
core/src/main/scala/org/apache/spark/network/netty/NettyBlockRpcServer.scala:
##########
@@ -93,8 +94,8 @@ class NettyBlockRpcServer(
} else {
val startAndEndId = fetchShuffleBlocks.reduceIds(index)
if (startAndEndId.length != 2) {
- throw new IllegalStateException(s"Invalid shuffle fetch request when batch mode " +
- s"is enabled: $fetchShuffleBlocks")
+ throw SparkException.internalError(s"Invalid shuffle fetch request when batch mode " +
+ s"is enabled: $fetchShuffleBlocks", category = "INTERNAL_ERROR_NETWORK")
Review Comment:
Just `NETWORK`, I think
```suggestion
s"is enabled: $fetchShuffleBlocks", category = "NETWORK")
```
##########
core/src/main/scala/org/apache/spark/network/netty/NettyBlockRpcServer.scala:
##########
@@ -93,8 +94,8 @@ class NettyBlockRpcServer(
} else {
val startAndEndId = fetchShuffleBlocks.reduceIds(index)
if (startAndEndId.length != 2) {
- throw new IllegalStateException(s"Invalid shuffle fetch request when batch mode " +
- s"is enabled: $fetchShuffleBlocks")
+ throw SparkException.internalError(s"Invalid shuffle fetch request when batch mode " +
Review Comment:
```suggestion
throw SparkException.internalError("Invalid shuffle fetch request when batch mode " +
```
##########
core/src/main/scala/org/apache/spark/network/netty/NettyBlockRpcServer.scala:
##########
@@ -137,13 +140,15 @@ class NettyBlockRpcServer(
val errorMsg = "Invalid GetLocalDirsForExecutors request: " +
s"${if (isIncorrectAppId) s"incorrect application id: ${getLocalDirs.appId};"}" +
s"${if (execNum != 1) s"incorrect executor number: $execNum (expected 1);"}"
- responseContext.onFailure(new IllegalStateException(errorMsg))
+ responseContext.onFailure(
+ SparkException.internalError(errorMsg, "INTERNAL_ERROR_NETWORK"))
} else {
val expectedExecId = blockManager.asInstanceOf[BlockManager].executorId
val actualExecId = getLocalDirs.execIds.head
if (actualExecId != expectedExecId) {
- responseContext.onFailure(new IllegalStateException(
- s"Invalid executor id: $actualExecId, expected $expectedExecId."))
+ responseContext.onFailure(SparkException.internalError(
+ s"Invalid executor id: $actualExecId, expected $expectedExecId.",
+ "INTERNAL_ERROR_NETWORK"))
Review Comment:
```suggestion
category = "NETWORK"))
```
##########
core/src/main/scala/org/apache/spark/network/netty/NettyBlockRpcServer.scala:
##########
@@ -137,13 +140,15 @@ class NettyBlockRpcServer(
val errorMsg = "Invalid GetLocalDirsForExecutors request: " +
s"${if (isIncorrectAppId) s"incorrect application id: ${getLocalDirs.appId};"}" +
s"${if (execNum != 1) s"incorrect executor number: $execNum (expected 1);"}"
- responseContext.onFailure(new IllegalStateException(errorMsg))
+ responseContext.onFailure(
+ SparkException.internalError(errorMsg, "INTERNAL_ERROR_NETWORK"))
Review Comment:
```suggestion
SparkException.internalError(errorMsg, category = "NETWORK"))
```
##########
core/src/main/scala/org/apache/spark/network/netty/NettyBlockRpcServer.scala:
##########
@@ -125,8 +126,10 @@ class NettyBlockRpcServer(
if (blockStored) {
responseContext.onSuccess(ByteBuffer.allocate(0))
} else {
- val exception = new Exception(s"Upload block for $blockId failed. This mostly happens " +
- s"when there is not sufficient space available to store the block.")
+ val exception = SparkException.internalError(
+ s"Upload block for $blockId failed. This mostly happens " +
+ s"when there is not sufficient space available to store the block.",
Review Comment:
s is not needed here
```suggestion
"when there is not sufficient space available to store the block.",
```
##########
core/src/main/scala/org/apache/spark/network/netty/NettyBlockRpcServer.scala:
##########
@@ -125,8 +126,10 @@ class NettyBlockRpcServer(
if (blockStored) {
responseContext.onSuccess(ByteBuffer.allocate(0))
} else {
- val exception = new Exception(s"Upload block for $blockId failed. This mostly happens " +
- s"when there is not sufficient space available to store the block.")
+ val exception = SparkException.internalError(
+ s"Upload block for $blockId failed. This mostly happens " +
+ s"when there is not sufficient space available to store the block.",
+ category = "INTERNAL_ERROR_NETWORK")
Review Comment:
```suggestion
category = "NETWORK")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on pull request #41140: [SPARK-38469][CORE] Use error class in org.apache.spark.network
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #41140:
URL: https://github.com/apache/spark/pull/41140#issuecomment-1549633131
@bozhang2820 Could you rebase on the recent master and re-trigger GAs, please.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on pull request #41140: [SPARK-38469][CORE] Use error class in org.apache.spark.network
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #41140:
URL: https://github.com/apache/spark/pull/41140#issuecomment-1549971413
Looking at the two last commit, seems like just flaky tests.
Merging to master. Thank you, @bozhang2820.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org