[SPARK-25576][BUILD][BRANCH-2.2] Fix lint failure by samdvr · Pull Request #22596 · apache/spark

samdvr · 2018-09-30T15:44:32Z

What changes were proposed in this pull request?

Line length fixes and

How was this patch tested?

Manually verified, but will ensure jenkins lint passes before merging

Related Job:
https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Compile/job/spark-branch-2.2-lint/913/console

HyukjinKwon · 2018-10-01T01:33:30Z

ok to test

HyukjinKwon · 2018-10-01T01:37:49Z

Can you link the JIRA https://issues.apache.org/jira/browse/SPARK-25576 ? Please see https://spark.apache.org/contributing.html

SparkQA · 2018-10-01T04:35:49Z

Test build #96808 has finished for PR 22596 at commit c4b6920.

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

samdvr · 2018-10-01T12:20:59Z

@HyukjinKwon linked the JIRA to this PR and tests have passed.

dongjoon-hyun · 2018-10-01T17:17:41Z

Thank you for your first contribution, @samdvr . Could you update the title, [SPARK-25576][BUILD][BRANCH-2.2] Fix Java Lint failure?

dongjoon-hyun · 2018-10-01T17:19:33Z

core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorter.java

-      // setting the array to null also indicates that it has already been de-allocated which prevents a double de-allocation in free().
+      // which in turn access this instance and eventually re-enter this method 
+      // and try to free the array again.
+      // by setting the array to null and its length to 0 


by -> By?

dongjoon-hyun · 2018-10-01T17:20:11Z

core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorter.java

+      // and try to free the array again.
+      // by setting the array to null and its length to 0 
+      // we effectively make the spill code-path a no-op.
+      // setting the array to null also indicates that it has already been 


setting -> Setting?

samdvr · 2018-10-01T17:27:25Z

@dongjoon-hyun updated per your comments, thank you.

dongjoon-hyun

Actually, this issue aims to pass dev/lint-java. For now, this PR seems to be unable to pass the Jenkins. Could you check it by run dev/lint-java in your spark home directory?

$ dev/lint-java
exec: curl --progress-bar -L https://downloads.typesafe.com/zinc/0.3.11/zinc-0.3.11.tgz
######################################################################## 100.0%
exec: curl --progress-bar -L https://downloads.typesafe.com/scala/2.11.8/scala-2.11.8.tgz
######################################################################## 100.0%
Using `mvn` from path: /usr/local/bin/mvn
Checkstyle checks failed at following occurrences:
[ERROR] src/test/java/org/apache/spark/unsafe/types/UTF8StringSuite.java:[66] (misc) AvoidEscapedUnicodeCharacters: Unicode escape(s) usage should be avoided.
[ERROR] src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorter.java:[177] (regexp) RegexpSingleline: No trailing whitespace allowed.
[ERROR] src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorter.java:[179] (regexp) RegexpSingleline: No trailing whitespace allowed.
[ERROR] src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorter.java:[181] (regexp) RegexpSingleline: No trailing whitespace allowed.
[ERROR] src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorterSuite.java:[473] (regexp) RegexpSingleline: No trailing whitespace allowed.
[ERROR] src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorterSuite.java:[478] (regexp) RegexpSingleline: No trailing whitespace allowed.
[ERROR] src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorterSuite.java:[485] (regexp) RegexpSingleline: No trailing whitespace allowed.
[ERROR] src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorterSuite.java:[489] (sizes) LineLength: Line is longer than 100 characters (found 120).
[ERROR] src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorterSuite.java:[491] (sizes) LineLength: Line is longer than 100 characters (found 114).
[ERROR] src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorterSuite.java:[186] (sizes) LineLength: Line is longer than 100 characters (found 116).

SparkQA · 2018-10-01T20:26:34Z

Test build #96823 has finished for PR 22596 at commit 009d4cf.

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

SparkQA · 2018-10-02T05:02:59Z

Test build #96835 has finished for PR 22596 at commit 2e8836b.

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

dongjoon-hyun

I made a PR to you, @samdvr . Could you merge that, samdvr#1 ?

dongjoon-hyun · 2018-10-02T06:22:29Z

common/unsafe/src/test/java/org/apache/spark/unsafe/types/UTF8StringSuite.java

    checkBasic("hello", 5); // 5 * 1 byte chars
    checkBasic("大 千 世 界", 7);
    checkBasic("︽﹋％", 3); // 3 * 3 bytes chars
-    checkBasic("\uD83E\uDD19", 1); // 4 bytes char


We should not remove test coverage. The correct fix is in my PR.

fix

samdvr · 2018-10-02T13:09:31Z

@dongjoon-hyun merged, thank you, did not know how to skip lint rule there.

SparkQA · 2018-10-02T16:10:13Z

Test build #96858 has finished for PR 22596 at commit 07156d4.

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

dongjoon-hyun

+1, LGTM. Thank you, @samdvr .
Merged to branch-2.2.

## What changes were proposed in this pull request? Line length fixes and ## How was this patch tested? Manually verified, but will ensure jenkins lint passes before merging Related Job: https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Compile/job/spark-branch-2.2-lint/913/console Closes #22596 from samdvr/SPARK-25576. Lead-authored-by: Sam Davarnia <sdavarnia@apple.com> Co-authored-by: Sam Davarnia <> Co-authored-by: Dongjoon Hyun <dongjoon@apache.org> Co-authored-by: Sam Davarnia <samdvr@users.noreply.github.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

dongjoon-hyun · 2018-10-02T17:22:12Z

BTW, I chose the most official one, Sam Davarnia <sdavarnia@apple.com>. But, it seems that your GitHub is not happy with that. Next time, you had better be consistent in your commit message.

Lead-authored-by: Sam Davarnia <sdavarnia@apple.com>
Co-authored-by: Sam Davarnia <>
Co-authored-by: Dongjoon Hyun <dongjoon@apache.org>
Co-authored-by: Sam Davarnia <samdvr@users.noreply.github.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

dongjoon-hyun · 2018-10-02T17:22:46Z

lint-java passed at 914.

Since it's merged into branch-2.2, could you close this PR? GitHub only closes the PRs against master branch. :)

samdvr · 2018-10-02T17:45:57Z

Thanks will do!

## What changes were proposed in this pull request? Line length fixes and ## How was this patch tested? Manually verified, but will ensure jenkins lint passes before merging Related Job: https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Compile/job/spark-branch-2.2-lint/913/console Closes apache#22596 from samdvr/SPARK-25576. Lead-authored-by: Sam Davarnia <sdavarnia@apple.com> Co-authored-by: Sam Davarnia <> Co-authored-by: Dongjoon Hyun <dongjoon@apache.org> Co-authored-by: Sam Davarnia <samdvr@users.noreply.github.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

fix liniting errors

c4b6920

dongjoon-hyun reviewed Oct 1, 2018

View reviewed changes

samdvr changed the title ~~Fix lint failure in 2.2~~ [SPARK-25576][BUILD][BRANCH-2.2] Fix lint failure Oct 1, 2018

pr comments

009d4cf

dongjoon-hyun requested changes Oct 1, 2018

View reviewed changes

lint fixes

2e8836b

fix

ff619cd

dongjoon-hyun requested changes Oct 2, 2018

View reviewed changes

dongjoon-hyun reviewed Oct 2, 2018

View reviewed changes

Merge pull request #1 from dongjoon-hyun/PR-22596

07156d4

fix

dongjoon-hyun approved these changes Oct 2, 2018

View reviewed changes

samdvr closed this Oct 2, 2018

Conversation

samdvr commented Sep 30, 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!

HyukjinKwon commented Oct 1, 2018

Uh oh!

HyukjinKwon commented Oct 1, 2018

Uh oh!

SparkQA commented Oct 1, 2018

Uh oh!

samdvr commented Oct 1, 2018

Uh oh!

dongjoon-hyun commented Oct 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun Oct 1, 2018

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Oct 1, 2018

Choose a reason for hiding this comment

Uh oh!

samdvr commented Oct 1, 2018

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 1, 2018

Uh oh!

SparkQA commented Oct 2, 2018

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Oct 2, 2018 • 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!

samdvr commented Oct 2, 2018

Uh oh!

SparkQA commented Oct 2, 2018

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Oct 2, 2018

Uh oh!

dongjoon-hyun commented Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samdvr commented Oct 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

samdvr commented Sep 30, 2018 •

edited

Loading

dongjoon-hyun commented Oct 1, 2018 •

edited

Loading

dongjoon-hyun Oct 2, 2018 •

edited

Loading

dongjoon-hyun commented Oct 2, 2018 •

edited

Loading