[GitHub] [spark] gengliangwang commented on a diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path

[GitBox <gi...@apache.org>]

gengliangwang commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r890373656


##########
core/src/main/java/org/apache/spark/memory/SparkOutOfMemoryError.java:
##########
@@ -28,6 +28,7 @@
 @Private
 public final class SparkOutOfMemoryError extends OutOfMemoryError implements SparkThrowable {
     String errorClass;
+    String errorSubClass;

Review Comment:
   It seems that the `errorSubClass` here is always null if there is no constructor method setting it.



##########
core/src/main/java/org/apache/spark/SparkThrowable.java:
##########
@@ -35,6 +35,9 @@ public interface SparkThrowable {
   // Succinct, human-readable, unique, and consistent representation of the error category
   // If null, error class is not set
   String getErrorClass();
+  default String getErrorSubClass() {

Review Comment:
   ```suggestion
   
       default String getErrorSubClass() {
   ```



##########
core/src/main/java/org/apache/spark/SparkThrowable.java:
##########
@@ -46,4 +49,13 @@ default String getSqlState() {
   default boolean isInternalError() {
     return SparkThrowableHelper.isInternalError(this.getErrorClass());
   }
+
+  default String[] getMessageParameters() {
+    return new String[]{};
+  }
+
+  // True if this error is an internal error.

Review Comment:
   We need to update the comment since it returns an array



##########
core/src/main/java/org/apache/spark/SparkThrowable.java:
##########
@@ -35,6 +35,9 @@ public interface SparkThrowable {
   // Succinct, human-readable, unique, and consistent representation of the error category
   // If null, error class is not set
   String getErrorClass();
+  default String getErrorSubClass() {

Review Comment:
   nit: Usually there needs a blank line between methods.



##########
core/src/main/java/org/apache/spark/SparkThrowable.java:
##########
@@ -35,6 +35,9 @@ public interface SparkThrowable {
   // Succinct, human-readable, unique, and consistent representation of the error category
   // If null, error class is not set
   String getErrorClass();
+  default String getErrorSubClass() {
+    return null;

Review Comment:
   Do we consider making the default value as empty string to avoid nullpointerexception?



-- 
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