[SPARK-35958][CORE] Refactor SparkError.scala to SparkThrowable.java #33164

karenfeng · 2021-07-01T03:31:09Z

What changes were proposed in this pull request?

Refactors the base Throwable trait SparkError.scala (introduced in SPARK-34920) an interface SparkThrowable.java.

Why are the changes needed?

Renaming SparkError to SparkThrowable better reflect sthat this is the base interface for both Exception and Error
Migrating to Java maximizes its extensibility

Does this PR introduce any user-facing change?

Yes; the base trait has been renamed and the accessor methods have changed (eg. sqlState -> getSqlState()).

How was this patch tested?

Unit tests.

Signed-off-by: Karen Feng <karen.feng@databricks.com>

cloud-fan · 2021-07-01T03:35:20Z

core/src/main/scala/org/apache/spark/SparkError.scala

- *   parameters to construct an error message with SparkError.getMessage(). New exception types
- *   should not accept arbitrary error messages. See [[SparkArithmeticException]].
- */
-trait SparkError extends Throwable {


I think we only to move this to java? Other parts are internal impl and can still be in scala.

Good call, it'll minimize the surface area of these changes

cloud-fan · 2021-07-01T03:36:49Z

core/src/test/java/org/apache/spark/SparkThrowableSuite.java

+
+import org.apache.spark.SparkThrowable.SparkThrowableHelper;
+
+public class SparkThrowableSuite implements Serializable {


I think the test in Scala is good enough, which also covers the most common case: we usually extend SparkThrowable in the scala code (spark codebase is mostly scala).

Done - I also added a check on the Java side

SparkQA · 2021-07-01T03:39:50Z

Test build #140478 has finished for PR 33164 at commit ef5db1e.

This patch fails RAT tests.
This patch merges cleanly.
This patch adds no public classes.

sigmod · 2021-07-01T04:18:41Z

core/src/main/java/org/apache/spark/SparkThrowable.java

+    String getSqlState();
+    String[] getMessageParameters();
+
+    class SparkThrowableHelper {


Does it have to be an inner class?

Moved back to Scala!

sigmod · 2021-07-01T04:19:42Z

core/src/main/java/org/apache/spark/SparkThrowable.java

+ *   types should not accept arbitrary error messages. See [[SparkArithmeticException]].
+ */
+public interface SparkThrowable {
+    String getErrorClass();


Add comments for interface methods?

Done - these generally reflect the README

sigmod · 2021-07-01T04:21:59Z

core/src/main/java/org/apache/spark/SparkThrowable.java

+            return null;
+        }
+
+        static SortedMap<String, ErrorInfo> getErrorClassToInfoMap() {


Moved back to Scala

sigmod · 2021-07-01T04:23:27Z

core/src/test/java/org/apache/spark/SparkThrowableSuite.java

@@ -0,0 +1,150 @@
+package org.apache.spark;


copy right header?

Moved back to Scala

SparkQA · 2021-07-01T04:39:14Z

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44990/

Signed-off-by: Karen Feng <karen.feng@databricks.com>

SparkQA · 2021-07-01T06:52:00Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44994/

SparkQA · 2021-07-01T07:28:17Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44994/

SparkQA · 2021-07-01T08:04:38Z

Test build #140482 has finished for PR 33164 at commit ec89069.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
throw new IllegalArgumentException(s\"Cannot find error class '$errorClass'\"))

Signed-off-by: Karen Feng <karen.feng@databricks.com>

SparkQA · 2021-07-01T19:25:03Z

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45046/

SparkQA · 2021-07-01T20:35:58Z

Test build #140533 has finished for PR 33164 at commit ec111f4.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Signed-off-by: Karen Feng <karen.feng@databricks.com>

SparkQA · 2021-07-01T21:43:35Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45052/

SparkQA · 2021-07-01T22:17:50Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45052/

SparkQA · 2021-07-01T23:44:15Z

Test build #140539 has finished for PR 33164 at commit 5db91e5.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2021-07-02T05:26:08Z

core/src/main/java/org/apache/spark/SparkThrowable.java

+ *   types should not accept arbitrary error messages. See [[SparkArithmeticException]].
+ */
+public interface SparkThrowable {
+    // Succinct, human-readable, unique, and consistent representation of the error category


Shall we use java Option? or document the null semantic and say that null means no error class?

I tried using Java's Optional, but it turns out that Scala's Option is quite different and we'd have to do wrappers with null conversions. I think using null is cleaner overall; I'll document this.

Yeah, I think you should wrap or manually convert that to Option .. but I guess that's fine though? are there many changes required for that?

core/src/main/resources/error/error-classes.json

core/src/main/java/org/apache/spark/memory/SparkOutOfMemoryError.java

Signed-off-by: Karen Feng <karen.feng@databricks.com>

SparkQA · 2021-07-06T20:45:09Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45226/

SparkQA · 2021-07-06T21:18:00Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45226/

SparkQA · 2021-07-06T22:24:11Z

Test build #140715 has finished for PR 33164 at commit f359e48.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Signed-off-by: Karen Feng <karen.feng@databricks.com>

HyukjinKwon · 2021-07-07T03:45:43Z

core/src/main/java/org/apache/spark/SparkThrowable.java

+ *   message with a null error class. See [[SparkException]].
+ * - To promote standardization, Throwables should be thrown with an error class and message
+ *   parameters to construct an error message with SparkThrowableHelper.getMessage(). New Throwable
+ *   types should not accept arbitrary error messages. See [[SparkArithmeticException]].


Shall we add @since?

Done! I also added @Evolving because it's so new.

HyukjinKwon · 2021-07-07T03:45:56Z

core/src/main/java/org/apache/spark/SparkThrowable.java

+ */
+@Experimental
+public interface SparkThrowable {
+    // Succinct, human-readable, unique, and consistent representation of the error category


Let's keep them 2-spaced

I know many of other java files are not but strictly it should be 2-spaced

Done (I think) - can you confirm?

SparkQA · 2021-07-07T03:50:11Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45239/

SparkQA · 2021-07-07T04:25:50Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45239/

SparkQA · 2021-07-07T05:27:39Z

Test build #140729 has finished for PR 33164 at commit d6f93b7.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Signed-off-by: Karen Feng <karen.feng@databricks.com>

core/src/main/resources/error/README.md

cloud-fan · 2021-07-07T17:26:02Z

core/src/main/scala/org/apache/spark/SparkException.scala

-  def this(errorClass: String, messageParameters: Seq[String]) =
-    this(errorClass = Some(errorClass), messageParameters = messageParameters)
+  override def getErrorClass: String = errorClass
+  override def getMessageParameters: Array[String] = messageParameters


It seems like we are totally fine if SparkThrowable does not have getMessageParameters. I think the message parameter is an internal thing and we shouldn't expose it to the end-users via a public API.

Good call - this was in order to construct a ParseException from an AnalysisException, but I was able to work around this by making messageParameters accessible as a val.

cloud-fan

LGTM except for some minor comments

Signed-off-by: Karen Feng <karen.feng@databricks.com>

SparkQA · 2021-07-07T17:55:19Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45272/

SparkQA · 2021-07-07T18:00:25Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45273/

SparkQA · 2021-07-07T18:27:50Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45272/

SparkQA · 2021-07-07T18:35:34Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45273/

SparkQA · 2021-07-07T18:53:52Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45274/

SparkQA · 2021-07-07T19:14:45Z

Test build #140761 has finished for PR 33164 at commit c2f0b86.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-07-07T19:30:02Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45274/

SparkQA · 2021-07-07T20:30:08Z

Test build #140762 has finished for PR 33164 at commit 048adf6.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2021-07-08T04:58:35Z

Looks good to me 2. the test failures in yarn cluster look unrelated.

### What changes were proposed in this pull request? Refactors the base Throwable trait `SparkError.scala` (introduced in SPARK-34920) an interface `SparkThrowable.java`. ### Why are the changes needed? - Renaming `SparkError` to `SparkThrowable` better reflect sthat this is the base interface for both `Exception` and `Error` - Migrating to Java maximizes its extensibility ### Does this PR introduce _any_ user-facing change? Yes; the base trait has been renamed and the accessor methods have changed (eg. `sqlState` -> `getSqlState()`). ### How was this patch tested? Unit tests. Closes #33164 from karenfeng/SPARK-35958. Authored-by: Karen Feng <karen.feng@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 71c086e) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

cloud-fan · 2021-07-08T15:55:56Z

thanks, merging to master/3.2!

karenfeng added 3 commits June 30, 2021 19:57

Migrate SparkError.scala to SparkThrowable.java

ef5db1e

Signed-off-by: Karen Feng <karen.feng@databricks.com>

Add Java demo

2d6ac78

Signed-off-by: Karen Feng <karen.feng@databricks.com>

Update README

fe117aa

Signed-off-by: Karen Feng <karen.feng@databricks.com>

karenfeng changed the title ~~[SPARK-35958] Refactor SparkError.scala to SparkThrowable.java~~ [SPARK-35958][CORE] Refactor SparkError.scala to SparkThrowable.java Jul 1, 2021

github-actions bot added CORE DOCS SQL labels Jul 1, 2021

cloud-fan reviewed Jul 1, 2021

View reviewed changes

sigmod reviewed Jul 1, 2021

View reviewed changes

Move code back to Scala

ec89069

Signed-off-by: Karen Feng <karen.feng@databricks.com>

sigmod approved these changes Jul 1, 2021

View reviewed changes

Test fixup

ec111f4

Signed-off-by: Karen Feng <karen.feng@databricks.com>

Retrigger tests

5db91e5

Signed-off-by: Karen Feng <karen.feng@databricks.com>

cloud-fan reviewed Jul 2, 2021

View reviewed changes

core/src/main/resources/error/error-classes.json Outdated Show resolved Hide resolved

gengliangwang reviewed Jul 2, 2021

View reviewed changes

core/src/main/java/org/apache/spark/memory/SparkOutOfMemoryError.java Outdated Show resolved Hide resolved

Address comments

f359e48

Signed-off-by: Karen Feng <karen.feng@databricks.com>

Modify PR template and add Experimental tag

d6f93b7

Signed-off-by: Karen Feng <karen.feng@databricks.com>

github-actions bot added the INFRA label Jul 7, 2021

HyukjinKwon reviewed Jul 7, 2021

View reviewed changes

karenfeng added 2 commits July 7, 2021 10:12

Mark as @evolving with @SInCE

fecf957

Signed-off-by: Karen Feng <karen.feng@databricks.com>

Remove unused @SInCE import

c2f0b86

Signed-off-by: Karen Feng <karen.feng@databricks.com>

cloud-fan reviewed Jul 7, 2021

View reviewed changes

core/src/main/resources/error/README.md Outdated Show resolved Hide resolved

cloud-fan reviewed Jul 7, 2021

View reviewed changes

cloud-fan approved these changes Jul 7, 2021

View reviewed changes

Remove getMessageParameters fn and clean up README

048adf6

Signed-off-by: Karen Feng <karen.feng@databricks.com>

cloud-fan closed this in 71c086e Jul 8, 2021


		import org.apache.spark.SparkThrowable.SparkThrowableHelper;

		public class SparkThrowableSuite implements Serializable {

[SPARK-35958][CORE] Refactor SparkError.scala to SparkThrowable.java #33164

[SPARK-35958][CORE] Refactor SparkError.scala to SparkThrowable.java #33164

Conversation

karenfeng commented Jul 1, 2021

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan Jul 1, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jul 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jul 1, 2021

SparkQA commented Jul 1, 2021

SparkQA commented Jul 1, 2021

SparkQA commented Jul 1, 2021

SparkQA commented Jul 1, 2021

SparkQA commented Jul 1, 2021

SparkQA commented Jul 1, 2021

SparkQA commented Jul 1, 2021

SparkQA commented Jul 1, 2021

Choose a reason for hiding this comment

karenfeng Jul 6, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jul 6, 2021

SparkQA commented Jul 6, 2021

SparkQA commented Jul 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jul 7, 2021

SparkQA commented Jul 7, 2021

SparkQA commented Jul 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan left a comment

Choose a reason for hiding this comment

SparkQA commented Jul 7, 2021

SparkQA commented Jul 7, 2021

SparkQA commented Jul 7, 2021

SparkQA commented Jul 7, 2021

SparkQA commented Jul 7, 2021

SparkQA commented Jul 7, 2021

SparkQA commented Jul 7, 2021

SparkQA commented Jul 7, 2021

HyukjinKwon commented Jul 8, 2021

cloud-fan commented Jul 8, 2021

cloud-fan Jul 1, 2021 •

edited

karenfeng Jul 6, 2021 •

edited