[SPARK-13583][CORE][STREAMING] Remove unused imports and add checkstyle rule by dongjoon-hyun · Pull Request #11438 · apache/spark

dongjoon-hyun · 2016-02-29T23:39:41Z

What changes were proposed in this pull request?

After SPARK-6990, dev/lint-java keeps Java code healthy and helps PR review by saving much time.
This issue aims remove unused imports from Java/Scala code and add UnusedImports checkstyle rule to help developers.

How was this patch tested?

./dev/lint-java
./build/sbt compile

srowen · 2016-02-29T23:44:47Z

I'm OK with this. Can we enforce this in Scala too?

Since we've bothered to fail the build if imports aren't in the right order I think it's reasonable to enforce removal of unused imports everywhere too.

@dongjoon-hyun I think you could add that too if you feel so inclined. If you're busy I might add it to https://issues.apache.org/jira/browse/SPARK-13423 but I like your focused PR here.

srowen · 2016-02-29T23:49:09Z

Jenkins, test this please

dongjoon-hyun · 2016-02-29T23:59:27Z

Thank you, @srowen !
Up to know, I'm not sure about enforcing Scala Imports but I'll check that, too.
I'll leave a message soon.

By the way, your SPARK-13423 looks great.

dongjoon-hyun · 2016-03-01T00:17:07Z

Hi, @srowen .
Surprisingly, according to the ScalaStyle Ruleset , it seems that we can not do that for Scala as of today. I think it's strange, do you think is there any other ways to do that?

zsxwing · 2016-03-01T00:18:07Z

FYI because mvn checkstyle:check depends on mvn install which cost huge time for sbt, java style checker is disabled now: https://github.com/apache/spark/blob/master/dev/run-tests.py#L546

dongjoon-hyun · 2016-03-01T00:53:08Z

Oh, thank you for informing that, @zsxwing .

By the way, dev\lint-java does not depend on mvn install. I think It depends on compiled classes.
You can do that by the following. How do you think of that, @zsxwing ? Is there any way to re-enable that?

$ ./build/sbt compile
$ # Add "<module name="UnusedImports"/>" into checkstyle.xml.
$ time ./dev/lint-java
...
real 0m14.880s
user 0m35.663s
sys 0m1.741s

dongjoon-hyun · 2016-03-01T00:55:27Z

@srowen , for the scala code, should we take advantage of -Ywarn-unused-import option for that in some way?

zsxwing · 2016-03-01T01:00:36Z

@dongjoon-hyun I mean lint-java will fail on a clean machine without maven cache. It will fail to resolve the snapshot jars which don't exist before running mvn install.

dongjoon-hyun · 2016-03-01T01:02:34Z

Oh, I see. Then, this PR will not enforce automatically, just helps developers to check before pushing PR.

Thank you, @zsxwing !

dongjoon-hyun · 2016-03-01T01:34:54Z

Hi, @srowen .
I've tested more, but found that -Ywarn-unused-import option is also not reliable yet in Scala 2.11.7.
Please refer https://issues.scala-lang.org/browse/SI-9616 .
I think I can not handle Scala part in this PR.
Sorry about that.

SparkQA · 2016-03-01T04:02:32Z

Test build #52203 has finished for PR 11438 at commit 5e82490.

This patch fails from timeout after a configured wait of 250m.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2016-03-01T04:33:05Z

It seems that Jenkins fails due to irrelevant things like the following.

Error instrumenting class:org.apache.spark.mllib.regression.IsotonicRegressionModel$SaveLoadV1_0$
...

Other PRs' test fail with similar logs. Should we wait for a while and re-trigger to test?

dongjoon-hyun · 2016-03-01T06:09:15Z

Rebased to trigger the Jenkins test.

srowen · 2016-03-01T10:56:11Z

If you're willing, I think you're welcome to also remove unused Scala imports, even if you can't enforce it. IDEs can make this easy.

Yes the test failure looks unrelated.

srowen · 2016-03-01T10:56:16Z

Jenkins, retest this please

SparkQA · 2016-03-01T15:07:28Z

Test build #52233 has finished for PR 11438 at commit 93b50e2.

This patch fails from timeout after a configured wait of 250m.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2016-03-01T20:05:28Z

Thank you for the testing, @srowen . Unfortunately, it still fails.
For the Scala Unused Imports, I see what you mean.
Then, I'll update Jira issue and remove them in this PR, too.
Thank you again.

zsxwing · 2016-03-01T20:11:20Z

ok to test

dongjoon-hyun · 2016-03-01T20:26:38Z

Thank you, @zsxwing !

SparkQA · 2016-03-02T00:22:40Z

Test build #52254 has finished for PR 11438 at commit 93b50e2.

This patch fails from timeout after a configured wait of 250m.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2016-03-02T08:59:13Z

Hi, @srowen .
The unused imports in Scala code are removed, too.

dongjoon-hyun · 2016-03-02T10:08:30Z

I've compared the Jenkins logs. My PR hangs at the following test.

[info] WithAggregationKinesisBackedBlockRDDSuite:
Using endpoint URL https://kinesis.us-west-2.amazonaws.com for creating Kinesis streams for tests.
[2016-03-01 13:06:10.815940] [0x00007fbb51998700] [info] [kinesis_producer.cc:79] Created pipeline for stream

Other PR passing this test looks like the following.

[info] WithAggregationKinesisBackedBlockRDDSuite:
[info] - Basic reading from Kinesis [enable by setting env var ENABLE_KINESIS_TESTS=1] !!! IGNORED !!!
[info] - Read data available in both block manager and Kinesis [enable by setting env var ENABLE_KINESIS_TESTS=1] !!! IGNORED !!!

Up to now, I cannot figure out how to make this PR pass the Jenkins. Could you give me some clue?

srowen · 2016-03-02T11:45:22Z

Yeah I think it's worth biting the bullet and cleaning up imports in one big go for 2.0.
The failures are certainly spurious, let's try again.

SparkQA · 2016-03-02T12:37:52Z

Test build #52313 has finished for PR 11438 at commit 627d561.

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

SparkQA · 2016-03-02T13:07:32Z

Test build #52310 has finished for PR 11438 at commit 90d0241.

This patch fails from timeout after a configured wait of 250m.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2016-03-02T16:42:54Z

Thank you. I see. I rebased to test one more time.

SparkQA · 2016-03-02T18:38:06Z

Test build #52323 has finished for PR 11438 at commit 1938445.

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

After SPARK-6990, `dev/lint-java` keeps Java code healthy and helps PR review by saving much time. This issue aims to enforce `UnusedImports` rule by adding a `UnusedImports` rule to `checkstyle.xml` and fixing all existing unused imports. ./build/sbt compile ./dev/lint-java

SparkQA · 2016-03-02T22:29:10Z

Test build #52333 has finished for PR 11438 at commit f85d167.

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

SparkQA · 2016-03-03T01:05:33Z

Test build #52340 has finished for PR 11438 at commit 4ecb8d7.

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

dongjoon-hyun · 2016-03-03T01:21:50Z

It passes the test finally. I updated the title and description of this PR and JIRA.
To sum up,

Remove all detected unused imports in Java (except one line in JavaKinesisStreamSuite.java)
Remove all observed unused imports in Scala (by manually with IntelliJ)
Add UnusedImports rule to checkstyle.xml

Thanks to your advice and help, I can finish this PR.
Thank you, @srowen and @zsxwing .

srowen · 2016-03-03T10:12:58Z

Merged to master

dongjoon-hyun · 2016-03-03T18:14:32Z

Thank you for merging!

…le rule ## What changes were proposed in this pull request? After SPARK-6990, `dev/lint-java` keeps Java code healthy and helps PR review by saving much time. This issue aims remove unused imports from Java/Scala code and add `UnusedImports` checkstyle rule to help developers. ## How was this patch tested? ``` ./dev/lint-java ./build/sbt compile ``` Author: Dongjoon Hyun <dongjoon@apache.org> Closesapache#11438 from dongjoon-hyun/SPARK-13583.

…rame decoder apache#12038 [EXT][SPARK-13583][CORE][STREAMING] Remove unused imports and add checkstyle rule apache#11438

dongjoon-hyun added 3 commits March 2, 2016 11:04

Change rule lines
af3c952

Remove unused imports in Scala code.
f85d167

Revert changes on JavaKinesisStreamSuite.java to skip Kinesis module
4ecb8d7

dongjoon-hyun changed the title ~~[SPARK-13583][BUILD] Enforce UnusedImports Java checkstyle rule~~[SPARK-13583][CORE][STREAMING] Remove unused imports and add checkstyle ruleMar 3, 2016

asfgit closed this in b5f02d6Mar 3, 2016

dongjoon-hyun deleted the SPARK-13583 branch March 3, 2016 20:41

dongjoon-hyun mentioned this pull request Mar 8, 2016
[HOT-FIX][BUILD] Use the new location of checkstyle-suppressions.xml#11567
Closed

zzcclp pushed a commit to zzcclp/spark that referenced this pull request Apr 1, 2016
[EXT][SPARK-14242][CORE][Network] avoid copy in compositeBuffer for f…
455edc5
…rame decoder apache#12038 [EXT][SPARK-13583][CORE][STREAMING] Remove unused imports and add checkstyle rule apache#11438

dongjoon-hyun mentioned this pull request May 10, 2016
[SPARK-15207][BUILD] Use Travis CI for Java Linter and JDK7/8 compilation test #12980
Closed

Uh oh!

Conversation

dongjoon-hyun commented Feb 29, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

srowen commented Feb 29, 2016

Uh oh!

srowen commented Feb 29, 2016

Uh oh!

dongjoon-hyun commented Feb 29, 2016

Uh oh!

dongjoon-hyun commented Mar 1, 2016

Uh oh!

zsxwing commented Mar 1, 2016

Uh oh!

dongjoon-hyun commented Mar 1, 2016

Uh oh!

dongjoon-hyun commented Mar 1, 2016

Uh oh!

zsxwing commented Mar 1, 2016

Uh oh!

dongjoon-hyun commented Mar 1, 2016

Uh oh!

dongjoon-hyun commented Mar 1, 2016

Uh oh!

SparkQA commented Mar 1, 2016

Uh oh!

dongjoon-hyun commented Mar 1, 2016

Uh oh!

dongjoon-hyun commented Mar 1, 2016

Uh oh!

srowen commented Mar 1, 2016

Uh oh!

srowen commented Mar 1, 2016

Uh oh!

SparkQA commented Mar 1, 2016

Uh oh!

dongjoon-hyun commented Mar 1, 2016

Uh oh!

zsxwing commented Mar 1, 2016

Uh oh!

dongjoon-hyun commented Mar 1, 2016

Uh oh!

SparkQA commented Mar 2, 2016

Uh oh!

dongjoon-hyun commented Mar 2, 2016

Uh oh!

dongjoon-hyun commented Mar 2, 2016

Uh oh!

srowen commented Mar 2, 2016

Uh oh!

SparkQA commented Mar 2, 2016

Uh oh!

SparkQA commented Mar 2, 2016

Uh oh!

dongjoon-hyun commented Mar 2, 2016

Uh oh!

SparkQA commented Mar 2, 2016

Uh oh!

SparkQA commented Mar 2, 2016

Uh oh!

SparkQA commented Mar 3, 2016

Uh oh!

dongjoon-hyun commented Mar 3, 2016

Uh oh!

srowen commented Mar 3, 2016

Uh oh!

dongjoon-hyun commented Mar 3, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants