Skip to content

Conversation

@dongjoon-hyun
Copy link
Member

@dongjoon-hyun dongjoon-hyun commented Apr 23, 2016

What changes were proposed in this pull request?

Spark uses NewLineAtEofChecker rule in Scala by ScalaStyle. And, most Java code also comply with the rule. This PR aims to enforce the same rule NewlineAtEndOfFile by CheckStyle explicitly. Also, this fixes lint-java errors since SPARK-14465. The followings are the items.

  • Adds a new line at the end of the files (19 files)
  • Fixes 25 lint-java errors (12 RedundantModifier, 6 ArrayTypeStyle, 2 LineLength, 2 UnusedImports, 2 RegexpSingleline, 1 ModifierOrder)

How was this patch tested?

After the Jenkins test succeeds, dev/lint-java should pass. (Currently, Jenkins dose not run lint-java.)

$ dev/lint-java 
Using `mvn` from path: /usr/local/bin/mvn
Checkstyle checks passed.

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-14868] Enable NewLineAtEofChecker in checkstyle and fix lint-java errors [SPARK-14868][BUILD] Enable NewLineAtEofChecker in checkstyle and fix lint-java errors Apr 23, 2016
@SparkQA
Copy link

SparkQA commented Apr 23, 2016

Test build #56783 has finished for PR 12632 at commit d20e167.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • public abstract static class RadixSortSupport extends PrefixComparator

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why can't this be final?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, it's definitely final. It's just RedundantModifier error since the class SignedPrefixComparator is already final.

@dongjoon-hyun
Copy link
Member Author

Hi, @rxin . Thank you for review. FYI, here is the result of dev/lint-java of current master branch.

spark:master$ dev/lint-java 
Using `mvn` from path: /usr/local/bin/mvn
Checkstyle checks failed at following occurrences:
[ERROR] src/main/java/org/apache/spark/shuffle/sort/ShuffleExternalSorter.java:[259] (sizes) LineLength: Line is longer than 100 characters (found 103).
[ERROR] src/main/java/org/apache/spark/util/collection/unsafe/sort/PrefixComparators.java:[25,8] (imports) UnusedImports: Unused import - org.apache.spark.util.Utils.
[ERROR] src/main/java/org/apache/spark/util/collection/unsafe/sort/PrefixComparators.java:[72,17] (modifier) ModifierOrder: 'abstract' modifier out of order with the JLS suggestions.
[ERROR] src/main/java/org/apache/spark/util/collection/unsafe/sort/PrefixComparators.java:[85,22] (modifier) RedundantModifier: Redundant 'final' modifier.
[ERROR] src/main/java/org/apache/spark/util/collection/unsafe/sort/PrefixComparators.java:[86,22] (modifier) RedundantModifier: Redundant 'final' modifier.
[ERROR] src/main/java/org/apache/spark/util/collection/unsafe/sort/PrefixComparators.java:[88,12] (modifier) RedundantModifier: Redundant 'final' modifier.
[ERROR] src/main/java/org/apache/spark/util/collection/unsafe/sort/PrefixComparators.java:[94,22] (modifier) RedundantModifier: Redundant 'final' modifier.
[ERROR] src/main/java/org/apache/spark/util/collection/unsafe/sort/PrefixComparators.java:[95,22] (modifier) RedundantModifier: Redundant 'final' modifier.
[ERROR] src/main/java/org/apache/spark/util/collection/unsafe/sort/PrefixComparators.java:[97,12] (modifier) RedundantModifier: Redundant 'final' modifier.
[ERROR] src/main/java/org/apache/spark/util/collection/unsafe/sort/PrefixComparators.java:[103,22] (modifier) RedundantModifier: Redundant 'final' modifier.
[ERROR] src/main/java/org/apache/spark/util/collection/unsafe/sort/PrefixComparators.java:[104,22] (modifier) RedundantModifier: Redundant 'final' modifier.
[ERROR] src/main/java/org/apache/spark/util/collection/unsafe/sort/PrefixComparators.java:[106,12] (modifier) RedundantModifier: Redundant 'final' modifier.
[ERROR] src/main/java/org/apache/spark/util/collection/unsafe/sort/PrefixComparators.java:[112,22] (modifier) RedundantModifier: Redundant 'final' modifier.
[ERROR] src/main/java/org/apache/spark/util/collection/unsafe/sort/PrefixComparators.java:[113,22] (modifier) RedundantModifier: Redundant 'final' modifier.
[ERROR] src/main/java/org/apache/spark/util/collection/unsafe/sort/PrefixComparators.java:[115,12] (modifier) RedundantModifier: Redundant 'final' modifier.
[ERROR] src/main/java/org/apache/spark/util/collection/unsafe/sort/RadixSort.java:[19] (regexp) RegexpSingleline: No trailing whitespace allowed.
[ERROR] src/main/java/org/apache/spark/util/collection/unsafe/sort/RadixSort.java:[230] (regexp) RegexpSingleline: No trailing whitespace allowed.
[ERROR] src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java:[215] (sizes) LineLength: Line is longer than 100 characters (found 103).
[ERROR] src/main/java/org/apache/spark/sql/execution/datasources/parquet/SpecificParquetRecordReaderBase.java:[41,8] (imports) UnusedImports: Unused import - org.apache.hadoop.fs.FileSystem.
[ERROR] src/test/java/org/apache/spark/ml/classification/JavaRandomForestClassifierSuite.java:[84,26] (misc) ArrayTypeStyle: Array brackets at illegal position.
[ERROR] src/test/java/org/apache/spark/ml/classification/JavaRandomForestClassifierSuite.java:[88,29] (misc) ArrayTypeStyle: Array brackets at illegal position.
[ERROR] src/test/java/org/apache/spark/ml/classification/JavaRandomForestClassifierSuite.java:[92,29] (misc) ArrayTypeStyle: Array brackets at illegal position.
[ERROR] src/test/java/org/apache/spark/ml/regression/JavaRandomForestRegressorSuite.java:[84,26] (misc) ArrayTypeStyle: Array brackets at illegal position.
[ERROR] src/test/java/org/apache/spark/ml/regression/JavaRandomForestRegressorSuite.java:[88,29] (misc) ArrayTypeStyle: Array brackets at illegal position.
[ERROR] src/test/java/org/apache/spark/ml/regression/JavaRandomForestRegressorSuite.java:[92,29] (misc) ArrayTypeStyle: Array brackets at illegal position.

@rxin
Copy link
Contributor

rxin commented Apr 24, 2016

Can we turn lint-java on for Jenkins in this pr?

@dongjoon-hyun
Copy link
Member Author

Sure! It's just one line change. May I turn it one right now?

@SparkQA
Copy link

SparkQA commented Apr 24, 2016

Test build #56837 has finished for PR 12632 at commit ac2880e.

  • This patch fails Java style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@dongjoon-hyun
Copy link
Member Author

Interesting. @rxin . Jenkins is trying to use Maven 3.1.1 due to the mismatch between --force option and lint-java.

Using `mvn` from path: /home/jenkins/tools/hudson.tasks.Maven_MavenInstallation/Maven_3.1.1/bin/mvn
Java HotSpot(TM) 64-Bit Server VM warning: ignoring option MaxPermSize=512M; support was removed in 8.0
Checkstyle checks failed at following occurrences:
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:2.17:check (default-cli) on project spark-parent_2.11: Unable to parse configuration of mojo org.apache.maven.plugins:maven-checkstyle-plugin:2.17:check for parameter sourceDirectories: Cannot assign configuration entry 'sourceDirectories' with value '/home/jenkins/workspace/SparkPullRequestBuilder/src/main/java,/home/jenkins/workspace/SparkPullRequestBuilder/src/main/scala' of type java.lang.String to property of type java.util.List -> [Help 1]

Actually, my PR #12631 aims to handle that bug. Could you review that too?

@dongjoon-hyun
Copy link
Member Author

I reverted the last commit about Jenkins Java Linter.

@SparkQA
Copy link

SparkQA commented Apr 24, 2016

Test build #56838 has finished for PR 12632 at commit a1747f3.

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

@dongjoon-hyun
Copy link
Member Author

It seems irrelevant errors. I'll rebase this to trigger again.

[info] Exception encountered when attempting to run a suite with class name: org.apache.spark.streaming.MapWithStateSuite *** ABORTED *** (41 seconds, 463 milliseconds)
[info]   java.io.IOException: Failed to delete: /home/jenkins/workspace/SparkPullRequestBuilder/streaming/checkpoint/spark-24a10e81-b717-4a88-b482-26b274b19f29
[info]   at org.apache.spark.util.Utils$.deleteRecursively(Utils.scala:938)

@SparkQA
Copy link

SparkQA commented Apr 24, 2016

Test build #56856 has finished for PR 12632 at commit c918d97.

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

@dongjoon-hyun
Copy link
Member Author

Since the failed tests are disabled at 0c8e533, I rebased again.

@SparkQA
Copy link

SparkQA commented Apr 25, 2016

Test build #56864 has finished for PR 12632 at commit 57885d2.

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

@dongjoon-hyun
Copy link
Member Author

Hi, @rxin .
I'm not sure when we can turn on Jenkins Linter again.
Could you merge this PR without that?

@rxin
Copy link
Contributor

rxin commented Apr 25, 2016

Merging in master.

@asfgit asfgit closed this in d34d650 Apr 25, 2016
@dongjoon-hyun
Copy link
Member Author

Thank you, @rxin .
For the Java Linter, I'll keep in mind and try to fix in another way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants