[SPARK-24529][Build][test-maven] Add spotbugs into maven build process #21542

kiszk · 2018-06-12T17:49:09Z

What changes were proposed in this pull request?

This PR enables a Java bytecode check tool spotbugs to avoid possible integer overflow at multiplication. When an violation is detected, the build process is stopped.
Due to the tool limitation, some other checks will be enabled. In this PR, these patterns in FindPuzzlers can be detected.

This check is enabled at compile phase. Thus, mvn compile or mvn package launches this check.

How was this patch tested?

Existing UTs

SparkQA · 2018-06-12T22:18:17Z

Test build #91720 has finished for PR 21542 at commit 3a356ad.

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

SparkQA · 2018-06-15T20:03:12Z

Test build #91923 has finished for PR 21542 at commit 9cb534e.

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

kiszk · 2018-06-15T21:24:22Z

After merging #21481, I confirmed build/mvn -DskipTests -T 8 clean compile does not find any violation by SpotBugs.

cc @cloud-fan @HyukjinKwon

cloud-fan · 2018-06-15T22:27:28Z

mllib/src/main/scala/org/apache/spark/ml/image/HadoopUtils.scala

is it a real problem?

It is not a real problem. However, I cannot find to disable only this check in findPuzzles.

cloud-fan · 2018-06-15T22:27:47Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

what does it mean?

It means x % 2 == does not work correctly if x is negative.

In this case, we know value.length is always non-negative . The current SpotBugs cannot detect this pre-condition. I cannot find to disable only this check in findPuzzles, too.

Therefore, this is a workaround to avoid the false positive violation.

HyukjinKwon · 2018-06-18T11:15:13Z

Sounds a good idea if what we all need is few changes like this. cc @srowen and @JoshRosen too who I believe might be interested in this too

srowen

I don't have the code in front of me; is there a findbugs config in Spark? We can remove it if so after this.

I am OK with adding static analysis like this.

srowen · 2018-06-18T13:51:43Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala

Are we sure this Comparator never returns Int.MinValue? If it can, then this is still a legitimate possible bug.

Good catch. We cannot ensure it. ordering.compare(o2, o1) looks better.

SparkQA · 2018-06-20T04:18:32Z

Test build #92112 has finished for PR 21542 at commit 75c9339.

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

kiszk · 2018-06-20T05:21:22Z

retest this please

SparkQA · 2018-06-20T07:05:01Z

Test build #92116 has finished for PR 21542 at commit 75c9339.

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

kiszk · 2018-06-20T08:15:40Z

retest this please

SparkQA · 2018-06-20T11:57:22Z

Test build #92125 has finished for PR 21542 at commit 75c9339.

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

HyukjinKwon · 2018-06-20T12:12:50Z

Hmm .. retest this please

SparkQA · 2018-06-20T15:51:58Z

Test build #92131 has finished for PR 21542 at commit 75c9339.

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

kiszk · 2018-06-20T16:01:14Z

retest this please

SparkQA · 2018-06-20T19:37:02Z

Test build #92139 has finished for PR 21542 at commit 75c9339.

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

kiszk · 2018-06-20T23:44:11Z

retest this please

SparkQA · 2018-06-21T00:54:37Z

Test build #92150 has finished for PR 21542 at commit 75c9339.

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

HyukjinKwon · 2018-06-21T01:45:33Z

Hmm .. seems roughly the error looks consistent.

kiszk · 2018-06-21T01:56:18Z

Unfortunately, looks consistent...

SparkQA · 2018-06-21T23:17:27Z

Test build #92180 has finished for PR 21542 at commit 86ef42c.

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

kiszk · 2018-06-22T02:20:52Z

Even when we stop forking SpotBugs, the same error occurred.
@HyukjinKwon is there any idea? I would appreciate your thoughts.

SparkQA · 2018-07-04T11:19:23Z

Test build #92601 has finished for PR 21542 at commit b027d62.

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

revert type

SparkQA · 2018-07-04T17:11:46Z

Test build #92609 has finished for PR 21542 at commit 191f135.

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

kiszk · 2018-07-04T19:42:05Z

cc @cloud-fan @viirya @HyukjinKwon

kiszk · 2018-07-04T19:43:43Z

resource-managers/kubernetes/core/pom.xml

      <scope>test</scope>
    </dependency>

+    <dependency>


This change is derived from the previous PR

kiszk · 2018-07-09T02:01:55Z

ping @cloud-fan @viirya @HyukjinKwon

viirya · 2018-07-09T02:08:42Z

pom.xml

+        <artifactId>spotbugs-maven-plugin</artifactId>
+        <version>3.1.3</version>
+        <configuration>
+          <classFilesDirectory>${basedir}/target/scala-2.11/classes</classFilesDirectory>


We may also want to apply it to 2.12 later?

yeah, me too. Good catch.

HyukjinKwon · 2018-07-09T02:10:31Z

Seems fine to me otherwise.

HyukjinKwon · 2018-07-09T02:20:55Z

pom.xml

+        <configuration>
+          <classFilesDirectory>${basedir}/target/scala-2.11/classes</classFilesDirectory>
+          <testClassFilesDirectory>${basedir}/target/scala-2.11/test-classes</testClassFilesDirectory>
+          <effort>Max</effort>


@kiszk, btw do you roughly know how much time this PR increases in the build?

I do not see large difference of maven's user time with and w/o Max

with Max

user 17m18.628s

without Max

user 17m3.724s

Eh, how much does it increase the Maven build time by this whole PR then roughly?

SparkQA · 2018-07-09T13:12:50Z

Test build #92746 has finished for PR 21542 at commit 6b747c3.

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

cloud-fan · 2018-07-09T13:52:06Z

retest this please

cloud-fan · 2018-07-09T13:52:27Z

LGTM, let's run test one more time to make sure it's not flaky

HyukjinKwon

LGTM too

But still it would be nicer if we know how much this PR one increases the total build time as well roughly since we actually take care about the build time.

SparkQA · 2018-07-09T17:46:38Z

Test build #92757 has finished for PR 21542 at commit 6b747c3.

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

kiszk · 2018-07-09T18:39:54Z

retest this please

kiszk · 2018-07-09T18:42:43Z

Does it make sense to compare these execution times thisPR and master

thisPR

4h20min building on an executor;

master

4h5min building on an executor;

SparkQA · 2018-07-09T23:01:23Z

Test build #92767 has finished for PR 21542 at commit 6b747c3.

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

viirya · 2018-07-09T23:07:58Z

LGTM too.

HyukjinKwon · 2018-07-10T01:06:57Z

Thank you so much @kiszk.

cloud-fan · 2018-07-11T15:48:23Z

sorry missed this thread...

Let me trigger test again to make sure new code merged during these 2 days do not violate the check.

jenkins retest this please

cloud-fan · 2018-07-11T15:51:02Z

retest this please

SparkQA · 2018-07-11T20:23:14Z

Test build #92865 has finished for PR 21542 at commit 6b747c3.

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

HyukjinKwon · 2018-07-12T01:52:01Z

Merged to master.

skonto · 2018-07-17T12:32:31Z

@HyukjinKwon @kiszk I see errors when I build locally on my machine:

[ERROR] Failed to execute goal com.github.spotbugs:spotbugs-maven-plugin:3.1.3:spotbugs (spotbugs) on project spark-network-common_2.11: Execution spotbugs of goal com.github.spotbugs:spotbugs-maven-plugin:3.1.3:spotbugs failed: Timeout: killed the sub-process -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/PluginExecutionException
[ERROR] 
[ERROR] After correcting the problems, you can resume the build with the command
[ERROR]   mvn <goals> -rf :spark-network-common_2.11
org.apache.tools.ant.ExitException: Permission ("java.lang.RuntimePermission" "exitVM") was not granted.
	at org.apache.tools.ant.types.Permissions$MySM.checkExit(Permissions.java:194)
	at java.lang.Runtime.exit(Runtime.java:107)
	at java.lang.System.exit(System.java:971)
	at org.codehaus.plexus.classworlds.launcher.Launcher.main(Launcher.java:358)
Exception in thread "main" org.apache.tools.ant.ExitException: Permission ("java.lang.RuntimePermission" "exitVM") was not granted.
	at org.apache.tools.ant.types.Permissions$MySM.checkExit(Permissions.java:194)
	at java.lang.Runtime.exit(Runtime.java:107)

cloud-fan · 2018-07-17T14:14:50Z

have you cleaned your environment? How exactly did you build Spark locally?

felixcheung · 2018-07-18T03:53:09Z

"permission" stuff might be Java 9 related?

HyukjinKwon · 2018-07-18T04:35:58Z

Hmmmm this happened to me too and I just pushed fc2e189. Mine was Java 8. There's reproducer in the PR description.

HyukjinKwon · 2018-07-25T02:27:36Z

This was reverted in favour of #21865 and SPARK-24895 for now.

kiszk force-pushed the SPARK-24529 branch from 3a356ad to 9cb534e Compare June 15, 2018 15:40

kiszk changed the title ~~[WIP][SPARK-24529][Build] Add spotbugs into maven build process~~ [SPARK-24529][Build] Add spotbugs into maven build process Jun 15, 2018

cloud-fan reviewed Jun 15, 2018

View reviewed changes

kiszk changed the title ~~[SPARK-24529][Build] Add spotbugs into maven build process~~ [SPARK-24529][Build][test-maven] Add spotbugs into maven build process Jun 17, 2018

srowen reviewed Jun 18, 2018

View reviewed changes

kiszk added 4 commits June 22, 2018 02:25

add spotbugs into pom.xml

4075f6f

avoid positive falses by SpotBugs

4441499

address review comments

02bbeb1

stop forking SpotBugs

86ef42c

kiszk force-pushed the SPARK-24529 branch from 75c9339 to 86ef42c Compare June 21, 2018 19:31

address review comment

191f135

revert type

kiszk commented Jul 4, 2018

View reviewed changes

viirya reviewed Jul 9, 2018

View reviewed changes

HyukjinKwon reviewed Jul 9, 2018

View reviewed changes

use scala.binary.version to specify directories

6b747c3

HyukjinKwon approved these changes Jul 9, 2018

View reviewed changes

asfgit closed this in 5ad4735 Jul 12, 2018

[SPARK-24529][Build][test-maven] Add spotbugs into maven build process #21542

[SPARK-24529][Build][test-maven] Add spotbugs into maven build process #21542

Uh oh!

Conversation

kiszk commented Jun 12, 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 Jun 12, 2018

Uh oh!

SparkQA commented Jun 15, 2018

Uh oh!

kiszk commented Jun 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

HyukjinKwon commented Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

SparkQA commented Jun 20, 2018

Uh oh!

kiszk commented Jun 20, 2018

Uh oh!

SparkQA commented Jun 20, 2018

Uh oh!

kiszk commented Jun 20, 2018

Uh oh!

SparkQA commented Jun 20, 2018

Uh oh!

HyukjinKwon commented Jun 20, 2018

Uh oh!

SparkQA commented Jun 20, 2018

Uh oh!

kiszk commented Jun 20, 2018

Uh oh!

SparkQA commented Jun 20, 2018

Uh oh!

kiszk commented Jun 20, 2018

Uh oh!

SparkQA commented Jun 21, 2018

Uh oh!

HyukjinKwon commented Jun 21, 2018

Uh oh!

kiszk commented Jun 21, 2018

Uh oh!

SparkQA commented Jun 21, 2018

Uh oh!

kiszk commented Jun 22, 2018

Uh oh!

SparkQA commented Jul 4, 2018

Uh oh!

SparkQA commented Jul 4, 2018

Uh oh!

kiszk commented Jul 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiszk commented Jul 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Jul 9, 2018

Uh oh!

kiszk commented Jun 12, 2018 •

edited

Loading

kiszk commented Jun 15, 2018 •

edited

Loading

HyukjinKwon commented Jun 18, 2018 •

edited

Loading

kiszk Jul 9, 2018 •

edited

Loading

cloud-fan commented Jul 9, 2018 •

edited

Loading

skonto commented Jul 17, 2018 •

edited

Loading