[SPARK-25986][Build] Add rules to ban throw Errors in application code #22989

xuanyuanking · 2018-11-09T05:26:15Z

What changes were proposed in this pull request?

Add scala and java lint check rules to ban the usage of throw new xxxErrors and fix up all exists instance followed by #22989 (comment). See more details in #22969.

How was this patch tested?

Local test with lint-scala and lint-java.

SparkQA · 2018-11-09T08:05:01Z

Test build #98637 has finished for PR 22989 at commit d678751.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

xuanyuanking · 2018-11-09T08:18:09Z

retest this please.

SparkQA · 2018-11-09T12:30:25Z

Test build #98649 has finished for PR 22989 at commit d678751.

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

scalastyle-config.xml

srowen

Where is it thrown directly though?

dev/checkstyle.xml

scalastyle-config.xml

srowen · 2018-11-09T15:30:57Z

Honestly it is bad style to throw any Error class, except in sepcial situations. I'd prefer a rule banning all of these and fix up any instances in the code or else exclude them

felixcheung · 2018-11-11T08:05:41Z

and catching Error or Throwable..

srowen · 2018-11-11T15:02:26Z

dev/checkstyle.xml

@@ -64,6 +64,11 @@
        <property name="message" value="No trailing whitespace allowed."/>
    </module>

+    <module name="RegexpSingleline">
+        <property name="format" value="throw new OutOfMemoryError"/>


Per my suggestion below, how about the regex throw new \\w+Error\\(

Thanks, done in 6e49bb8.

xuanyuanking · 2018-11-12T03:58:27Z

Sorry for late reply, great thanks for all reviewer's advise, will address them soon.

xuanyuanking · 2018-11-12T16:16:51Z

cc all reviewer, as @srowen's suggestion, add a rule to ban all of new Error cases.
List currently throw new XXXError in Spark source below and record fix up or exclude for review conveniently:

ErrorName	Count	In Test/Source Code	Fix Up or Exclude
AssertionError	32	29 in test code	Fix up by replacing to `fail()`
		2 in UnsafeAlignedOffset	legitimate, exclude them
		1 in KafkaUtils	Fix up by changing to `IllegalArgumentException`
NotImplementedError	22	7 in source code/ 15 in test cases	Fix up by changing them to `UnsupportedOperationException`
OutOfMemoryError	1	1 in test code	Exclude
LinkageError	1	1 in test code	Exclude
SparkOutOfMemoryError	5	5 in source code	Exclude
UnknownError	9	5 in source code/ 4 in test cases	Fix up by changing to `IllegalAccessException` and `IllegalArgumentException`

~~All above done in 6e49bb8.~~
Addressed Sean's suggestion, edit the above table and done in ff234d3.

SparkQA · 2018-11-12T16:31:08Z

Test build #98731 has finished for PR 22989 at commit 6e49bb8.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2018-11-12T17:06:59Z

AssertionError in tests is wrong. It should really just call fail(). If you're willing to fix that, it would be a great cleanup.

The AssertionError in UnsafeAlignedOffset is legitimate and can be excluded.

I would fix the AssertionError in KafkaUtils. That one is clearly an illegal argument check.

Agree about the rest. Thanks for the great analysis @xuanyuanking

xuanyuanking · 2018-11-13T04:02:22Z

@srowen Great thanks for your guidance, address all your suggestion in ff234d3 and update the record table in #22989 (comment).

srowen

Looks quite good to me, just a few comments.

srowen · 2018-11-13T07:09:46Z

dev/checkstyle-suppressions.xml

@@ -46,4 +46,12 @@
              files="sql/catalyst/src/main/java/org/apache/spark/sql/streaming/GroupStateTimeout.java"/>
    <suppress checks="MethodName"
              files="sql/core/src/main/java/org/apache/spark/sql/streaming/Trigger.java"/>
+    <suppress checks="RegexpSingleline"


I think this might suppress many checks? is it difficult to suppress with comments in these files?

Yep, I met some problem during suppress with comments, just work around by suppress files, I will find the problem again.

I found the problem is RegexpSingleline can't put into TreeWalker, so SuppressionCommentFilter rule can't effect on it, RegexpSinglelineJava is the right choice. Also spend some time on comment syntax, maybe the usage description can be improved a little, please have a check whether I understand this rule correctly. Done in a4f49ce.

mllib/src/test/scala/org/apache/spark/ml/feature/VectorIndexerSuite.scala

SparkQA · 2018-11-13T07:21:21Z

Test build #98749 has finished for PR 22989 at commit ff234d3.

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

SparkQA · 2018-11-13T14:36:14Z

Test build #98770 has finished for PR 22989 at commit a4f49ce.

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

mllib/src/test/scala/org/apache/spark/mllib/clustering/KMeansSuite.scala

viirya · 2018-11-14T05:30:15Z

dev/checkstyle.xml

@@ -180,5 +180,10 @@
        <module name="UnusedImports"/>
        <module name="RedundantImport"/>
        <module name="RedundantModifier"/>
+        <module name="RegexpSinglelineJava">
+            <property name="format" value="throw new \w+Error\("/>
+            <property name="message" value="Avoid throwing error in application code."/>


What application code means here?

application code against JVM here, as Error subclasses generally represent internal JVM errors.

SparkQA · 2018-11-14T17:10:18Z

Test build #98826 has finished for PR 22989 at commit 210d942.

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

srowen · 2018-11-14T21:05:46Z

Merged to master

xuanyuanking · 2018-11-15T01:56:53Z

Thanks @HyukjinKwon @viirya @felixcheung @srowen for your review and advise!

cloud-fan · 2018-11-15T04:05:24Z

common/unsafe/src/main/java/org/apache/spark/unsafe/UnsafeAlignedOffset.java

@@ -39,7 +39,9 @@ public static int getSize(Object object, long offset) {
      case 8:
        return (int)Platform.getLong(object, offset);
      default:
+        // checkstyle.off: RegexpSinglelineJava
        throw new AssertionError("Illegal UAO_SIZE");


shall we throw IllegalStateException here?

I think these are ok as AssertionError because they shouldn't be able to happen in any JVM state

yea, that's exactly the use case of IllegalStateException, which can also pass the style check here.

It would be OK too. This is just picking nits now but I thought AssertionError was still better. ISE is about application state. Errors are about JVM state.

cloud-fan · 2018-11-15T04:05:39Z

common/unsafe/src/main/java/org/apache/spark/unsafe/UnsafeAlignedOffset.java

@@ -52,7 +54,9 @@ public static void putSize(Object object, long offset, int value) {
        Platform.putLong(object, offset, value);
        break;
      default:
+        // checkstyle.off: RegexpSinglelineJava
        throw new AssertionError("Illegal UAO_SIZE");


## What changes were proposed in this pull request? Add scala and java lint check rules to ban the usage of `throw new xxxErrors` and fix up all exists instance followed by apache#22989 (comment). See more details in apache#22969. ## How was this patch tested? Local test with lint-scala and lint-java. Closes apache#22989 from xuanyuanking/SPARK-25986. Authored-by: Yuanjian Li <[email protected]> Signed-off-by: Sean Owen <[email protected]>

banning throw new OutOfMemoryErrors

d678751

viirya reviewed Nov 9, 2018

View reviewed changes

scalastyle-config.xml Outdated Show resolved Hide resolved

srowen reviewed Nov 9, 2018

View reviewed changes

dev/checkstyle.xml Outdated Show resolved Hide resolved

HyukjinKwon reviewed Nov 9, 2018

View reviewed changes

scalastyle-config.xml Outdated Show resolved Hide resolved

srowen reviewed Nov 11, 2018

View reviewed changes

Ban all of new Error cases

6e49bb8

Fix up AssertionError in test/UnsafeAlignedOffset and KafkaUtils

ff234d3

xuanyuanking changed the title ~~[SPARK-25986][Build] Banning throw new OutOfMemoryErrors~~ [SPARK-25986][Build] Add rules to ban throw Errors in application code Nov 13, 2018

srowen reviewed Nov 13, 2018

View reviewed changes

use RegexpSinglelineJava

a4f49ce

srowen approved these changes Nov 13, 2018

View reviewed changes

HyukjinKwon approved these changes Nov 14, 2018

View reviewed changes

viirya reviewed Nov 14, 2018

View reviewed changes

mllib/src/test/scala/org/apache/spark/mllib/clustering/KMeansSuite.scala Outdated Show resolved Hide resolved

viirya reviewed Nov 14, 2018

View reviewed changes

address viirya comments

210d942

asfgit closed this in 2977e23 Nov 14, 2018

xuanyuanking deleted the SPARK-25986 branch November 15, 2018 01:56

cloud-fan reviewed Nov 15, 2018

View reviewed changes

[SPARK-25986][Build] Add rules to ban throw Errors in application code #22989

[SPARK-25986][Build] Add rules to ban throw Errors in application code #22989

Uh oh!

Conversation

xuanyuanking commented Nov 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Nov 9, 2018

Uh oh!

xuanyuanking commented Nov 9, 2018

Uh oh!

SparkQA commented Nov 9, 2018

Uh oh!

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

srowen commented Nov 9, 2018

Uh oh!

felixcheung commented Nov 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xuanyuanking commented Nov 12, 2018

Uh oh!

xuanyuanking commented Nov 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Nov 12, 2018

Uh oh!

srowen commented Nov 12, 2018

Uh oh!

xuanyuanking commented Nov 13, 2018

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SparkQA commented Nov 13, 2018

Uh oh!

SparkQA commented Nov 13, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 14, 2018

Uh oh!

srowen commented Nov 14, 2018

Uh oh!

xuanyuanking commented Nov 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xuanyuanking commented Nov 9, 2018 •

edited

Loading

xuanyuanking commented Nov 12, 2018 •

edited

Loading