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/06/13 14:12:24 UTC
[GitHub] [spark] bozhang2820 opened a new pull request, #41575: [SPARK-38477][CORE] Use error class in org.apache.spark.shuffle
bozhang2820 opened a new pull request, #41575:
URL: https://github.com/apache/spark/pull/41575
### What changes were proposed in this pull request?
This PR aims to change exceptions created in package org.apache.spark.shuffle to use error class. This also adds an error class INTERNAL_ERROR_SHUFFLE 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.shuffle 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] MaxGekk commented on pull request #41575: [SPARK-38477][CORE] Use error class in org.apache.spark.shuffle
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #41575:
URL: https://github.com/apache/spark/pull/41575#issuecomment-1623606199
@Hisoka-X Could you make the assert message shorter, and print only `Error class: INTERNAL_ERROR_SHUFFLE is not in sql-error-conditions.md`, see https://github.com/bozhang2820/spark/actions/runs/5465673291/jobs/9949470464
--
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 a diff in pull request #41575: [SPARK-38477][CORE] Use error class in org.apache.spark.shuffle
Posted by "bozhang2820 (via GitHub)" <gi...@apache.org>.
bozhang2820 commented on code in PR #41575:
URL: https://github.com/apache/spark/pull/41575#discussion_r1254229702
##########
core/src/main/scala/org/apache/spark/errors/SparkCoreErrors.scala:
##########
@@ -467,6 +467,15 @@ private[spark] object SparkCoreErrors {
"receivedBytes" -> receivedBytes.toString).asJava)
}
+ def failedRenameTempFileError(srcFile: File, dstFile: File): Throwable = {
+ new SparkException(
+ errorClass = "_LEGACY_ERROR_TEMP_2199",
Review Comment:
Assigned a name to the error class.
--
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 a diff in pull request #41575: [SPARK-38477][CORE] Use error class in org.apache.spark.shuffle
Posted by "bozhang2820 (via GitHub)" <gi...@apache.org>.
bozhang2820 commented on code in PR #41575:
URL: https://github.com/apache/spark/pull/41575#discussion_r1252904610
##########
core/src/main/scala/org/apache/spark/errors/SparkCoreErrors.scala:
##########
@@ -467,6 +467,15 @@ private[spark] object SparkCoreErrors {
"receivedBytes" -> receivedBytes.toString).asJava)
}
+ def failedRenameTempFileError(srcFile: File, dstFile: File): Throwable = {
+ new SparkException(
+ errorClass = "_LEGACY_ERROR_TEMP_2199",
Review Comment:
`FAILED_RENAME_PATH` is for the case when the destination already exists. `_LEGACY_ERROR_TEMP_2199` suits our case better.
--
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 #41575: [SPARK-38477][CORE] Use error class in org.apache.spark.shuffle
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #41575: [SPARK-38477][CORE] Use error class in org.apache.spark.shuffle
URL: https://github.com/apache/spark/pull/41575
--
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 #41575: [SPARK-38477][CORE] Use error class in org.apache.spark.shuffle
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #41575:
URL: https://github.com/apache/spark/pull/41575#discussion_r1245469582
##########
core/src/main/scala/org/apache/spark/shuffle/IndexShuffleBlockResolver.scala:
##########
@@ -263,7 +264,8 @@ private[spark] class IndexShuffleBlockResolver(
file.delete()
}
if (!fileTmp.renameTo(file)) {
- throw new IOException(s"fail to rename file ${fileTmp} to ${file}")
+ throw SparkException.internalError(
Review Comment:
I think that re-using the existing one `SparkRuntimeException` with a proper error class should be enough but `SparkIOException` works to me as well.
--
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 a diff in pull request #41575: [SPARK-38477][CORE] Use error class in org.apache.spark.shuffle
Posted by "bozhang2820 (via GitHub)" <gi...@apache.org>.
bozhang2820 commented on code in PR #41575:
URL: https://github.com/apache/spark/pull/41575#discussion_r1254527968
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -724,6 +724,11 @@
],
"sqlState" : "42K04"
},
+ "FAILED_RENAME_TEMP_FILE" : {
+ "message" : [
+ "Failed to rename temp file <srcPath> to <dstPath> as rename returned false."
Review Comment:
Change to `FileSystem.rename`.
--
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 #41575: [SPARK-38477][CORE] Use error class in org.apache.spark.shuffle
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #41575:
URL: https://github.com/apache/spark/pull/41575#discussion_r1252952244
##########
core/src/main/scala/org/apache/spark/errors/SparkCoreErrors.scala:
##########
@@ -467,6 +467,15 @@ private[spark] object SparkCoreErrors {
"receivedBytes" -> receivedBytes.toString).asJava)
}
+ def failedRenameTempFileError(srcFile: File, dstFile: File): Throwable = {
+ new SparkException(
+ errorClass = "_LEGACY_ERROR_TEMP_2199",
Review Comment:
Can you assign a proper name?
--
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 #41575: [SPARK-38477][CORE] Use error class in org.apache.spark.shuffle
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #41575:
URL: https://github.com/apache/spark/pull/41575#discussion_r1254378816
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -724,6 +724,11 @@
],
"sqlState" : "42K04"
},
+ "FAILED_RENAME_TEMP_FILE" : {
+ "message" : [
+ "Failed to rename temp file <srcPath> to <dstPath> as rename returned false."
Review Comment:
Which rename returned false. Could you make it clearer? Do you mean rename of filesystem.
--
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 #41575: [SPARK-38477][CORE] Use error class in org.apache.spark.shuffle
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #41575:
URL: https://github.com/apache/spark/pull/41575#discussion_r1252899647
##########
core/src/main/scala/org/apache/spark/errors/SparkCoreErrors.scala:
##########
@@ -467,6 +467,15 @@ private[spark] object SparkCoreErrors {
"receivedBytes" -> receivedBytes.toString).asJava)
}
+ def failedRenameTempFileError(srcFile: File, dstFile: File): Throwable = {
+ new SparkException(
+ errorClass = "_LEGACY_ERROR_TEMP_2199",
Review Comment:
Can't you re-using the existing one `FAILED_RENAME_PATH`?
--
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 #41575: [SPARK-38477][CORE] Use error class in org.apache.spark.shuffle
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #41575:
URL: https://github.com/apache/spark/pull/41575#discussion_r1231944151
##########
core/src/main/scala/org/apache/spark/shuffle/IndexShuffleBlockResolver.scala:
##########
@@ -263,7 +264,8 @@ private[spark] class IndexShuffleBlockResolver(
file.delete()
}
if (!fileTmp.renameTo(file)) {
- throw new IOException(s"fail to rename file ${fileTmp} to ${file}")
+ throw SparkException.internalError(
Review Comment:
@bozhang2820 Are you sure that IO errors should be considered as internal errors? Users can face to them in real life, I guess.
--
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 #41575: [SPARK-38477][CORE] Use error class in org.apache.spark.shuffle
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #41575:
URL: https://github.com/apache/spark/pull/41575#issuecomment-1623789366
@bozhang2820 Could you resolve conflicts, 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] Hisoka-X commented on pull request #41575: [SPARK-38477][CORE] Use error class in org.apache.spark.shuffle
Posted by "Hisoka-X (via GitHub)" <gi...@apache.org>.
Hisoka-X commented on PR #41575:
URL: https://github.com/apache/spark/pull/41575#issuecomment-1623610114
> @Hisoka-X Could you make the assert message shorter, and print only `The error class INTERNAL_ERROR_SHUFFLE is not in sql-error-conditions.md`, see https://github.com/bozhang2820/spark/actions/runs/5465673291/jobs/9949470464
Ok, let me fix it.
--
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 #41575: [SPARK-38477][CORE] Use error class in org.apache.spark.shuffle
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #41575:
URL: https://github.com/apache/spark/pull/41575#issuecomment-1627707310
+1, LGTM. Merging to master.
Thank you, @bozhang2820 and @holdenk for review.
--
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 a diff in pull request #41575: [SPARK-38477][CORE] Use error class in org.apache.spark.shuffle
Posted by "bozhang2820 (via GitHub)" <gi...@apache.org>.
bozhang2820 commented on code in PR #41575:
URL: https://github.com/apache/spark/pull/41575#discussion_r1243878899
##########
core/src/main/scala/org/apache/spark/shuffle/IndexShuffleBlockResolver.scala:
##########
@@ -263,7 +264,8 @@ private[spark] class IndexShuffleBlockResolver(
file.delete()
}
if (!fileTmp.renameTo(file)) {
- throw new IOException(s"fail to rename file ${fileTmp} to ${file}")
+ throw SparkException.internalError(
Review Comment:
Sorry for the late reply. Yes I think we might need a `SparkIOException` which extends `IOException`?
--
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