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/23 09:33:51 UTC

[GitHub] [spark] bozhang2820 opened a new pull request, #41277: [SPARK-38464][CORE] Use error classes in org.apache.spark.io

bozhang2820 opened a new pull request, #41277:
URL: https://github.com/apache/spark/pull/41277

   
   ### What changes were proposed in this pull request?
   This PR aims to change exceptions created in package org.apache.spark.io to use error class.
   
   
   ### Why are the changes needed?
   This is to move exceptions created in package org.apache.spark.io to error class.
   
   
   ### Does this PR introduce _any_ user-facing change?
   No
   
   
   ### How was this patch tested?
   Updated 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 closed pull request #41277: [SPARK-38464][CORE] Use error classes in org.apache.spark.io

Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #41277: [SPARK-38464][CORE] Use error classes in org.apache.spark.io
URL: https://github.com/apache/spark/pull/41277


-- 
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 #41277: [SPARK-38464][CORE] Use error classes in org.apache.spark.io

Posted by "bozhang2820 (via GitHub)" <gi...@apache.org>.
bozhang2820 commented on code in PR #41277:
URL: https://github.com/apache/spark/pull/41277#discussion_r1203679166


##########
core/src/main/scala/org/apache/spark/io/CompressionCodec.scala:
##########
@@ -88,8 +88,12 @@ private[spark] object CompressionCodec {
     } catch {
       case _: ClassNotFoundException | _: IllegalArgumentException => None
     }
-    codec.getOrElse(throw new IllegalArgumentException(s"Codec [$codecName] is not available. " +
-      s"Consider setting $configKey=$FALLBACK_COMPRESSION_CODEC"))
+    codec.getOrElse(throw new SparkIllegalArgumentException(
+      errorClass = "CODEC_NOT_AVAILABLE",
+      messageParameters = Map(
+        "codecName" -> codecName,
+        "configKey" -> configKey,

Review Comment:
   Done.



-- 
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 #41277: [SPARK-38464][CORE] Use error classes in org.apache.spark.io

Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #41277:
URL: https://github.com/apache/spark/pull/41277#discussion_r1202422005


##########
core/src/main/resources/error/error-classes.json:
##########
@@ -187,6 +187,11 @@
     ],
     "sqlState" : "22003"
   },
+  "CODEC_NOT_AVAILABLE" : {
+    "message" : [
+      "Codec [<codecName>] is not available. Consider setting <configKey>=<configVal>"

Review Comment:
   nit:
   ```suggestion
         "The codec <codecName> is not available. Consider to set the config <configKey> to <configVal>."
   ```



##########
core/src/test/scala/org/apache/spark/io/CompressionCodecSuite.scala:
##########
@@ -127,9 +127,10 @@ class CompressionCodecSuite extends SparkFunSuite {
   }
 
   test("bad compression codec") {
-    intercept[IllegalArgumentException] {
+    val e = intercept[SparkIllegalArgumentException] {
       CompressionCodec.createCodec(conf, "foobar")
     }
+    assert(e.getErrorClass == "CODEC_NOT_AVAILABLE")

Review Comment:
   Please, use `checkError()`



##########
core/src/main/scala/org/apache/spark/io/CompressionCodec.scala:
##########
@@ -88,8 +88,12 @@ private[spark] object CompressionCodec {
     } catch {
       case _: ClassNotFoundException | _: IllegalArgumentException => None
     }
-    codec.getOrElse(throw new IllegalArgumentException(s"Codec [$codecName] is not available. " +
-      s"Consider setting $configKey=$FALLBACK_COMPRESSION_CODEC"))
+    codec.getOrElse(throw new SparkIllegalArgumentException(
+      errorClass = "CODEC_NOT_AVAILABLE",
+      messageParameters = Map(
+        "codecName" -> codecName,
+        "configKey" -> configKey,

Review Comment:
   Please, wrap it by `toSQLConf`



##########
core/src/main/scala/org/apache/spark/io/CompressionCodec.scala:
##########
@@ -88,8 +88,12 @@ private[spark] object CompressionCodec {
     } catch {
       case _: ClassNotFoundException | _: IllegalArgumentException => None
     }
-    codec.getOrElse(throw new IllegalArgumentException(s"Codec [$codecName] is not available. " +
-      s"Consider setting $configKey=$FALLBACK_COMPRESSION_CODEC"))
+    codec.getOrElse(throw new SparkIllegalArgumentException(
+      errorClass = "CODEC_NOT_AVAILABLE",
+      messageParameters = Map(
+        "codecName" -> codecName,
+        "configKey" -> configKey,
+        "configVal" -> FALLBACK_COMPRESSION_CODEC)))

Review Comment:
   Could you wrap it by `toSQLConfVal`



##########
core/src/main/resources/error/error-classes.json:
##########
@@ -1385,6 +1390,11 @@
       "No handler for UDAF '<functionName>'. Use sparkSession.udf.register(...) instead."
     ]
   },
+  "NO_SHORT_NAME_FOR_CODEC" : {
+    "message" : [
+      "No short name for codec <codecName>."

Review Comment:
   nit:
   ```suggestion
         "Not found a short name for the codec <codecName>."
   ```



##########
core/src/main/scala/org/apache/spark/io/CompressionCodec.scala:
##########
@@ -88,8 +88,12 @@ private[spark] object CompressionCodec {
     } catch {
       case _: ClassNotFoundException | _: IllegalArgumentException => None
     }
-    codec.getOrElse(throw new IllegalArgumentException(s"Codec [$codecName] is not available. " +
-      s"Consider setting $configKey=$FALLBACK_COMPRESSION_CODEC"))
+    codec.getOrElse(throw new SparkIllegalArgumentException(
+      errorClass = "CODEC_NOT_AVAILABLE",
+      messageParameters = Map(
+        "codecName" -> codecName,

Review Comment:
   Please, wrap it, for instance by `toSQLValue`



-- 
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 #41277: [SPARK-38464][CORE] Use error classes in org.apache.spark.io

Posted by "bozhang2820 (via GitHub)" <gi...@apache.org>.
bozhang2820 commented on code in PR #41277:
URL: https://github.com/apache/spark/pull/41277#discussion_r1203426212


##########
core/src/main/scala/org/apache/spark/io/CompressionCodec.scala:
##########
@@ -88,8 +88,12 @@ private[spark] object CompressionCodec {
     } catch {
       case _: ClassNotFoundException | _: IllegalArgumentException => None
     }
-    codec.getOrElse(throw new IllegalArgumentException(s"Codec [$codecName] is not available. " +
-      s"Consider setting $configKey=$FALLBACK_COMPRESSION_CODEC"))
+    codec.getOrElse(throw new SparkIllegalArgumentException(
+      errorClass = "CODEC_NOT_AVAILABLE",
+      messageParameters = Map(
+        "codecName" -> codecName,
+        "configKey" -> configKey,
+        "configVal" -> FALLBACK_COMPRESSION_CODEC)))

Review Comment:
   Done.



##########
core/src/main/scala/org/apache/spark/io/CompressionCodec.scala:
##########
@@ -88,8 +88,12 @@ private[spark] object CompressionCodec {
     } catch {
       case _: ClassNotFoundException | _: IllegalArgumentException => None
     }
-    codec.getOrElse(throw new IllegalArgumentException(s"Codec [$codecName] is not available. " +
-      s"Consider setting $configKey=$FALLBACK_COMPRESSION_CODEC"))
+    codec.getOrElse(throw new SparkIllegalArgumentException(
+      errorClass = "CODEC_NOT_AVAILABLE",
+      messageParameters = Map(
+        "codecName" -> codecName,
+        "configKey" -> configKey,

Review Comment:
   Ditto. Should we move these to spark-core? Or have them duplicated?



##########
core/src/main/scala/org/apache/spark/io/CompressionCodec.scala:
##########
@@ -88,8 +88,12 @@ private[spark] object CompressionCodec {
     } catch {
       case _: ClassNotFoundException | _: IllegalArgumentException => None
     }
-    codec.getOrElse(throw new IllegalArgumentException(s"Codec [$codecName] is not available. " +
-      s"Consider setting $configKey=$FALLBACK_COMPRESSION_CODEC"))
+    codec.getOrElse(throw new SparkIllegalArgumentException(
+      errorClass = "CODEC_NOT_AVAILABLE",
+      messageParameters = Map(
+        "codecName" -> codecName,

Review Comment:
   `toSQLValue` is defined in spark-sql, we cannot use it directly here in spark-core.



-- 
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 #41277: [SPARK-38464][CORE] Use error classes in org.apache.spark.io

Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #41277:
URL: https://github.com/apache/spark/pull/41277#discussion_r1203548606


##########
core/src/main/scala/org/apache/spark/io/CompressionCodec.scala:
##########
@@ -88,8 +88,12 @@ private[spark] object CompressionCodec {
     } catch {
       case _: ClassNotFoundException | _: IllegalArgumentException => None
     }
-    codec.getOrElse(throw new IllegalArgumentException(s"Codec [$codecName] is not available. " +
-      s"Consider setting $configKey=$FALLBACK_COMPRESSION_CODEC"))
+    codec.getOrElse(throw new SparkIllegalArgumentException(
+      errorClass = "CODEC_NOT_AVAILABLE",
+      messageParameters = Map(
+        "codecName" -> codecName,
+        "configKey" -> configKey,

Review Comment:
   Oh, sorry. Let's define in `core` at leats `quoteByDefault()`, `toConf`, and `toConfVal`.



-- 
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 #41277: [SPARK-38464][CORE] Use error classes in org.apache.spark.io

Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #41277:
URL: https://github.com/apache/spark/pull/41277#issuecomment-1560937976

   +1, LGTM. 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