[SPARK-18420][BUILD] Fix the errors caused by lint check in Java #15865

ConeyLiu · 2016-11-12T15:13:58Z

What changes were proposed in this pull request?

Small fix, fix the errors caused by lint check in Java

Clear unused objects and UnusedImports.
Add comments around the method finalize of NioBufferedFileInputStreamto turn off checkstyle.
Cut the line which is longer than 100 characters into two lines.

How was this patch tested?

Travis CI.

$ build/mvn -T 4 -q -DskipTests -Pyarn -Phadoop-2.3 -Pkinesis-asl -Phive -Phive-thriftserver install
$ dev/lint-java

Before:

Checkstyle checks failed at following occurrences:
[ERROR] src/main/java/org/apache/spark/network/util/TransportConf.java:[21,8] (imports) UnusedImports: Unused import - org.apache.commons.crypto.cipher.CryptoCipherFactory.
[ERROR] src/test/java/org/apache/spark/network/sasl/SparkSaslSuite.java:[516,5] (modifier) RedundantModifier: Redundant 'public' modifier.
[ERROR] src/main/java/org/apache/spark/io/NioBufferedFileInputStream.java:[133] (coding) NoFinalizer: Avoid using finalizer method.
[ERROR] src/main/java/org/apache/spark/sql/catalyst/expressions/UnsafeMapData.java:[71] (sizes) LineLength: Line is longer than 100 characters (found 113).
[ERROR] src/main/java/org/apache/spark/sql/catalyst/expressions/UnsafeArrayData.java:[112] (sizes) LineLength: Line is longer than 100 characters (found 110).
[ERROR] src/test/java/org/apache/spark/sql/catalyst/expressions/HiveHasherSuite.java:[31,17] (modifier) ModifierOrder: 'static' modifier out of order with the JLS suggestions.
[ERROR]src/main/java/org/apache/spark/examples/ml/JavaLogisticRegressionWithElasticNetExample.java:[64] (sizes) LineLength: Line is longer than 100 characters (found 103).
[ERROR] src/main/java/org/apache/spark/examples/ml/JavaInteractionExample.java:[22,8] (imports) UnusedImports: Unused import - org.apache.spark.ml.linalg.Vectors.
[ERROR] src/main/java/org/apache/spark/examples/ml/JavaInteractionExample.java:[51] (regexp) RegexpSingleline: No trailing whitespace allowed.

After:

$ build/mvn -T 4 -q -DskipTests -Pyarn -Phadoop-2.3 -Pkinesis-asl -Phive -Phive-thriftserver install
$ dev/lint-java
Using `mvn` from path: /home/travis/build/ConeyLiu/spark/build/apache-maven-3.3.9/bin/mvn
Checkstyle checks passed.

HyukjinKwon · 2016-11-12T19:39:28Z

Hi @ConeyLiu, It seems

[ERROR] src/main/java/org/apache/spark/io/NioBufferedFileInputStream.java:[133] (coding) NoFinalizer: Avoid using finalizer method.

is missed. If it is a required change, I guess we should disable the lint check for the lines.

ConeyLiu · 2016-11-13T01:42:37Z

@HyukjinKwon I'm sorry for that, I've fixed it, please take a look. Thanks a lot.

srowen · 2016-11-13T12:45:39Z

They're caused by lint right? not checkstyle? and these are only Java, because that's all that's not automatically tested. Please update the title/description with more meaningful details, including the JIRA.

srowen · 2016-11-13T12:43:48Z

core/src/main/java/org/apache/spark/io/NioBufferedFileInputStream.java

Why was this removed? it needs to be here for sure

srowen · 2016-11-13T12:44:44Z

sql/catalyst/src/test/java/org/apache/spark/sql/catalyst/expressions/HiveHasherSuite.java

You made this non-static; that's not a style issue right? it may be OK but some of these changes don't seem to match the description

@srowen I've updated, but It seems the hasher never used.

ConeyLiu · 2016-11-13T14:56:27Z

@srowen Thanks for the review, I've updated it.

HyukjinKwon · 2016-11-14T05:37:51Z

@ConeyLiu Let us please change the title to something like [SPARK-18420][BUILD] Fix the errors caused by lint check in Java for this pull request and JIRA you opened. Also, it might be great if the descriptions in both this pull request and JIRA have more details about this.

Also, I think for the one below:

[ERROR] src/main/java/org/apache/spark/io/NioBufferedFileInputStream.java:[133] (coding) NoFinalizer: Avoid using finalizer method.

we should disable the lint for the lines because it seems a required one. I guess you should disable the check for the lines or work around this.

Could we then remove hasher in HiveHasherSuite.java if it is not used? FWIW, I just checked that this private instance is not referred in HiveHasherSuite.java for sure.

Also, I have a link I usually refer, https://cwiki.apache.org/confluence/display/SPARK/Contributing+to+Spark

Maybe, this pull request and JIRA can be nicer if both follow the guild lines.

ConeyLiu · 2016-11-14T13:20:45Z

@HyukjinKwon Thanks for the review and suggestion, I've updated it. Clear the unused object hasher and add suppression rules for the method finalize of NioBufferedFileInputStream. Please take a look, thanks a lot.

HyukjinKwon · 2016-11-14T13:23:52Z

dev/checkstyle-suppressions.xml

Ah, I thought we could disable it only for some lines like Scala/Python/R. I am fine with this if it is only the way.

@HyukjinKwon Also we could try // style:off ... // style:on. I'll test this way.

SparkQA · 2016-11-14T16:46:35Z

Test build #3425 has finished for PR 15865 at commit 39c2a85.

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

HyukjinKwon · 2016-11-15T02:05:45Z

sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/UnsafeArrayData.java

Hi @ConeyLiu, it is a super minor but it seems we should make the indentation for this (and the same instances here) double-spaced. I at least could find some examples for this such as in UnsafeMemoryAllocator

assert (memory.obj == null) : "baseObject not null; are you trying to use the off-heap allocator to free on-heap memory?";

or in ShuffleSortDataFormat

assert (length <= buffer.size()) : "the buffer is smaller than required: " + buffer.size() + " < " + length;

HyukjinKwon · 2016-11-15T02:07:03Z

examples/src/main/java/org/apache/spark/examples/ml/JavaInteractionExample.java

If you are supposed to push more commits, let us revert this extra newline removal. I guess some guys don't like such changes. At least I was told twice before.

HyukjinKwon · 2016-11-15T02:07:46Z

For sure, I ran

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

It seems fine. It looks good to me for few minor comments above.

ConeyLiu · 2016-11-15T08:28:02Z

@HyukjinKwon I'm sorry for so late reply. I update the code and style followed by your suggestion, that's so useful. And also I change the method of disabling the check around finalize. Please take a loot or give some suggestion. Thanks a lot.

HyukjinKwon · 2016-11-15T08:51:23Z

dev/checkstyle.xml

+        <property name="onCommentFormat" value="checkstyle.on\: ([\w\|]+)"/>
+        <property name="checkFormat" value="$1"/>
+    </module>
+


Ah, I am sorry @ConeyLiu. I should have taken a look ahead first. I actually rather meant agreeing with the previous state if we should introduce such new property... Although I (as just one of contributors) think this might be worth, maybe this should be handled in a different PR.

Maybe, I think we could keep this change and then propose a new separate PR later if other people also agree with this.

Let us maybe please revert this change back and move back to the state which Sean approved for sure (but including the indentation fixes and newline fix). I will try to run the lint for sure as soon as you do.

Ok, maybe we need more advices.

Ah OK, this is needed in order to implement a checkstyle:off for Java like we use scalastyle:off for Scala? that's fine to do here if so, because it seems like we need it to get past the finalizer issue. I think that's a fine general mechanism to enable.

ConeyLiu · 2016-11-15T11:27:36Z

@srowen hi owen, Can you give me some advice, and if so, what a great thing.

srowen · 2016-11-15T14:51:11Z

dev/checkstyle.xml

        <module name="UnusedImports"/>
        <module name="RedundantImport"/>
        <module name="RedundantModifier"/>
+        <module name="FileContentsHolder"/>


What is this needed for, the checkstyle:off mechanism?

@srowen SuppressionCommentFilterand FileContentsHolder must work together,http://stackoverflow.com/questions/5761188/checkstyle-suppressioncommentfilter-not-ignoring-specified-rule

HyukjinKwon · 2016-11-15T15:24:45Z

I just checked for sure.

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

ConeyLiu · 2016-11-16T00:00:33Z

@HyukjinKwon @srowen Thank you very much.

Small fix, fix the errors caused by lint check in Java - Clear unused objects and `UnusedImports`. - Add comments around the method `finalize` of `NioBufferedFileInputStream`to turn off checkstyle. - Cut the line which is longer than 100 characters into two lines. Travis CI. ``` $ build/mvn -T 4 -q -DskipTests -Pyarn -Phadoop-2.3 -Pkinesis-asl -Phive -Phive-thriftserver install $ dev/lint-java ``` Before: ``` Checkstyle checks failed at following occurrences: [ERROR] src/main/java/org/apache/spark/network/util/TransportConf.java:[21,8] (imports) UnusedImports: Unused import - org.apache.commons.crypto.cipher.CryptoCipherFactory. [ERROR] src/test/java/org/apache/spark/network/sasl/SparkSaslSuite.java:[516,5] (modifier) RedundantModifier: Redundant 'public' modifier. [ERROR] src/main/java/org/apache/spark/io/NioBufferedFileInputStream.java:[133] (coding) NoFinalizer: Avoid using finalizer method. [ERROR] src/main/java/org/apache/spark/sql/catalyst/expressions/UnsafeMapData.java:[71] (sizes) LineLength: Line is longer than 100 characters (found 113). [ERROR] src/main/java/org/apache/spark/sql/catalyst/expressions/UnsafeArrayData.java:[112] (sizes) LineLength: Line is longer than 100 characters (found 110). [ERROR] src/test/java/org/apache/spark/sql/catalyst/expressions/HiveHasherSuite.java:[31,17] (modifier) ModifierOrder: 'static' modifier out of order with the JLS suggestions. [ERROR]src/main/java/org/apache/spark/examples/ml/JavaLogisticRegressionWithElasticNetExample.java:[64] (sizes) LineLength: Line is longer than 100 characters (found 103). [ERROR] src/main/java/org/apache/spark/examples/ml/JavaInteractionExample.java:[22,8] (imports) UnusedImports: Unused import - org.apache.spark.ml.linalg.Vectors. [ERROR] src/main/java/org/apache/spark/examples/ml/JavaInteractionExample.java:[51] (regexp) RegexpSingleline: No trailing whitespace allowed. ``` After: ``` $ build/mvn -T 4 -q -DskipTests -Pyarn -Phadoop-2.3 -Pkinesis-asl -Phive -Phive-thriftserver install $ dev/lint-java Using `mvn` from path: /home/travis/build/ConeyLiu/spark/build/apache-maven-3.3.9/bin/mvn Checkstyle checks passed. ``` Author: Xianyang Liu <[email protected]> Closes #15865 from ConeyLiu/master. (cherry picked from commit 7569cf6) Signed-off-by: Sean Owen <[email protected]>

srowen · 2016-11-16T12:49:22Z

Merged to master/2.1

## What changes were proposed in this pull request? Small fix, fix the errors caused by lint check in Java - Clear unused objects and `UnusedImports`. - Add comments around the method `finalize` of `NioBufferedFileInputStream`to turn off checkstyle. - Cut the line which is longer than 100 characters into two lines. ## How was this patch tested? Travis CI. ``` $ build/mvn -T 4 -q -DskipTests -Pyarn -Phadoop-2.3 -Pkinesis-asl -Phive -Phive-thriftserver install $ dev/lint-java ``` Before: ``` Checkstyle checks failed at following occurrences: [ERROR] src/main/java/org/apache/spark/network/util/TransportConf.java:[21,8] (imports) UnusedImports: Unused import - org.apache.commons.crypto.cipher.CryptoCipherFactory. [ERROR] src/test/java/org/apache/spark/network/sasl/SparkSaslSuite.java:[516,5] (modifier) RedundantModifier: Redundant 'public' modifier. [ERROR] src/main/java/org/apache/spark/io/NioBufferedFileInputStream.java:[133] (coding) NoFinalizer: Avoid using finalizer method. [ERROR] src/main/java/org/apache/spark/sql/catalyst/expressions/UnsafeMapData.java:[71] (sizes) LineLength: Line is longer than 100 characters (found 113). [ERROR] src/main/java/org/apache/spark/sql/catalyst/expressions/UnsafeArrayData.java:[112] (sizes) LineLength: Line is longer than 100 characters (found 110). [ERROR] src/test/java/org/apache/spark/sql/catalyst/expressions/HiveHasherSuite.java:[31,17] (modifier) ModifierOrder: 'static' modifier out of order with the JLS suggestions. [ERROR]src/main/java/org/apache/spark/examples/ml/JavaLogisticRegressionWithElasticNetExample.java:[64] (sizes) LineLength: Line is longer than 100 characters (found 103). [ERROR] src/main/java/org/apache/spark/examples/ml/JavaInteractionExample.java:[22,8] (imports) UnusedImports: Unused import - org.apache.spark.ml.linalg.Vectors. [ERROR] src/main/java/org/apache/spark/examples/ml/JavaInteractionExample.java:[51] (regexp) RegexpSingleline: No trailing whitespace allowed. ``` After: ``` $ build/mvn -T 4 -q -DskipTests -Pyarn -Phadoop-2.3 -Pkinesis-asl -Phive -Phive-thriftserver install $ dev/lint-java Using `mvn` from path: /home/travis/build/ConeyLiu/spark/build/apache-maven-3.3.9/bin/mvn Checkstyle checks passed. ``` Author: Xianyang Liu <[email protected]> Closes apache#15865 from ConeyLiu/master.

srowen requested changes Nov 13, 2016

View reviewed changes

ConeyLiu force-pushed the master branch from 11f17ab to 1bc737e Compare November 13, 2016 13:49

ConeyLiu changed the title ~~[SPARK-18420]Fix the compile errors caused by checkstyle~~ [SPARK-18420][SPARK][BUILD]Fix the compile errors caused by checkstyle Nov 13, 2016

ConeyLiu changed the title ~~[SPARK-18420][SPARK][BUILD]Fix the compile errors caused by checkstyle~~ [SPARK-18420][BUILD] Fix the errors caused by lint check in Java Nov 14, 2016

HyukjinKwon reviewed Nov 14, 2016

View reviewed changes

srowen approved these changes Nov 14, 2016

View reviewed changes

HyukjinKwon reviewed Nov 15, 2016

View reviewed changes

Xianyang Liu added 2 commits November 15, 2016 11:03

Fix the errors caused by lint check in Java

af2185c

Add comments around 'finalize' to turn off checkstyle.

0317f62

ConeyLiu force-pushed the master branch from 39c2a85 to 0317f62 Compare November 15, 2016 08:21

HyukjinKwon reviewed Nov 15, 2016

View reviewed changes

srowen reviewed Nov 15, 2016

View reviewed changes

srowen approved these changes Nov 15, 2016

View reviewed changes

asfgit closed this in 7569cf6 Nov 16, 2016

ConeyLiu deleted the master branch March 10, 2017 02:35

[SPARK-18420][BUILD] Fix the errors caused by lint check in Java #15865

[SPARK-18420][BUILD] Fix the errors caused by lint check in Java #15865

Uh oh!

Conversation

ConeyLiu commented Nov 12, 2016 • 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!

HyukjinKwon commented Nov 12, 2016

Uh oh!

ConeyLiu commented Nov 13, 2016

Uh oh!

srowen commented Nov 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ConeyLiu Nov 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ConeyLiu commented Nov 13, 2016

Uh oh!

HyukjinKwon commented Nov 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ConeyLiu commented Nov 14, 2016

Uh oh!

HyukjinKwon Nov 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ConeyLiu Nov 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Nov 15, 2016

Uh oh!

ConeyLiu commented Nov 15, 2016

Uh oh!

HyukjinKwon Nov 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ConeyLiu commented Nov 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Nov 15, 2016

Uh oh!

ConeyLiu commented Nov 16, 2016

Uh oh!

srowen commented Nov 16, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ConeyLiu commented Nov 12, 2016 •

edited

Loading

ConeyLiu Nov 13, 2016 •

edited

Loading

HyukjinKwon commented Nov 14, 2016 •

edited

Loading

HyukjinKwon Nov 14, 2016 •

edited

Loading

ConeyLiu Nov 14, 2016 •

edited

Loading

HyukjinKwon Nov 15, 2016 •

edited

Loading