[SPARK-13692][CORE][SQL] Fix trivial Coverity/Checkstyle defects #11530

dongjoon-hyun · 2016-03-04T23:49:15Z

What changes were proposed in this pull request?

This issue fixes the following potential bugs and Java coding style detected by Coverity and Checkstyle.

Implement both null and type checking in equals functions.
Fix wrong type casting logic in SimpleJavaBean2.equals.
Add implement Cloneable to UTF8String and SortedIterator.
Remove dereferencing before null check in AbstractBytesToBytesMapSuite.
Fix coding style: Add '{}' to single for statement in mllib examples.
Remove unused imports in ColumnarBatch and JavaKinesisStreamSuite.
Remove unused fields in ChunkFetchIntegrationSuite.
Add stop() to prevent resource leak.

Please note that the last two checkstyle errors exist on newly added commits after SPARK-13583.

How was this patch tested?

manual via ./dev/lint-java and Coverity site.

dongjoon-hyun · 2016-03-04T23:50:32Z

Hi, @srowen .
This is based on Coverity reports. Could you review this please?

srowen · 2016-03-05T08:35:24Z

examples/src/main/java/org/apache/spark/examples/mllib/JavaGaussianMixtureExample.java

@@ -44,8 +44,9 @@ public static void main(String[] args) {
        public Vector call(String s) {
          String[] sarray = s.trim().split(" ");
          double[] values = new double[sarray.length];
-          for (int i = 0; i < sarray.length; i++)
+          for (int i = 0; i < sarray.length; i++) {


This is fine, but it wasn't the analysis problem. A lot of the examples don't stop() the context at the end, and this was one of about 30.

Oh, I fix this since it's the error from one 'Checkstyle' (lint-java).
Up to now, this PR does not include stop() context discussed in emails.

Ah I see. That's important then.

srowen · 2016-03-05T08:39:55Z

It's looking fine; do you want to address some more of the simple ones? like identical branches of an if, unused fields, etc?

srowen · 2016-03-05T09:18:58Z

Jenkins test this please

dongjoon-hyun · 2016-03-05T10:48:58Z

Thank you, @srowen .
Since Coverity was announced in mailing lists, I made this PR/JIRA quickly to avoid any potential conflicts.
I'll try to add more and let you know when I'm finished.

SparkQA · 2016-03-05T11:50:54Z

Test build #52513 has finished for PR 11530 at commit 9f74e82.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- public final class SortedIterator extends UnsafeSorterIterator implements Cloneable

dongjoon-hyun · 2016-03-05T12:49:04Z

Hi, @srowen . I did the followings, too.

- Remove unused fields in `ChunkFetchIntegrationSuite`.
- Add `close()` to prevent resource leak.

Could you merge this if the test passes again?

dongjoon-hyun · 2016-03-05T12:51:22Z

retest this please

dongjoon-hyun · 2016-03-05T19:46:10Z

retest this please

dongjoon-hyun · 2016-03-05T23:30:52Z

retest this please

dongjoon-hyun · 2016-03-06T17:51:49Z

Hi, @srowen .
Sorry, but could you ask Jenkins to retest this PR?
I tried to ask Jenkins by asking 'retest this please' and by rebasing this PR, but it does not response for 24 hours.

srowen · 2016-03-06T20:12:30Z

streaming/src/test/java/org/apache/spark/streaming/JavaAPISuite.java

@@ -390,6 +390,7 @@ public void testQueueStream() {
    JavaTestUtils.attachTestOutputStream(stream);
    List<List<Integer>> result = JavaTestUtils.runStreams(ssc, 3, 3);
    Assert.assertEquals(expected, result);
+    jsc.close();


These types of changes are welcome of course but I think we need to call stop() in the examples, not close(). Or at least that's what the others do. I don't think Coverity will realize that, but that's fine, it's then a false positive we just resolve as such.

Oh, thank you for pointing that. I will replace them all with stop.

dongjoon-hyun · 2016-03-06T20:38:24Z

Hi, @srowen .
I update this PR and JIRA to use close() instead of stop().

srowen · 2016-03-07T10:01:05Z

Jenkins, test this please

SparkQA · 2016-03-07T14:12:32Z

Test build #52553 has finished for PR 11530 at commit bea31f9.

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-07T16:52:06Z

Oh, @srowen .
There is no apparent error for me. It stops before SparkR tests and marked.

Build step 'Execute shell' marked build as failure

It seems really timeout for the whole test.

Started 6 hr 47 min ago
Took 4 hr 11 min on amp-jenkins-worker-08

Do you think we put too much thing in one PR?

dongjoon-hyun · 2016-03-07T18:07:00Z

Just rebased.

dongjoon-hyun · 2016-03-07T22:42:10Z

Thanks to #11564 , I can fix JavaKinesisStreamSuite too.

dongjoon-hyun · 2016-03-07T22:54:38Z

retest this please

zsxwing · 2016-03-07T23:42:02Z

ok to test

dongjoon-hyun · 2016-03-07T23:45:01Z

Thank you, @zsxwing !

SparkQA · 2016-03-08T01:54:53Z

Test build #52611 has finished for PR 11530 at commit 9a0f8fa.

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

zsxwing · 2016-03-08T04:59:45Z

retest this please

SparkQA · 2016-03-08T07:20:55Z

Test build #52631 has finished for PR 11530 at commit 9a0f8fa.

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

dongjoon-hyun · 2016-03-08T07:29:10Z

Thank you, @zsxwing .

dongjoon-hyun · 2016-03-08T08:44:12Z

Hi, @srowen .
Could you merge this PR, please?
It includes JavaKinesisStreamSuite part, too.

srowen · 2016-03-08T10:52:00Z

...ples/src/main/java/org/apache/spark/examples/streaming/JavaTwitterHashTagJoinSentiments.java

@@ -171,5 +171,6 @@ public void call(JavaPairRDD<Double, String> happinessTopicPairs) {

    jssc.start();
    jssc.awaitTermination();
+    jssc.close();


Ah, I just realized I misled you in my comments. For streaming examples, we should not call close() or stop(). The correct last call is awaitTermination(). If you back those out, I'll merge this. Sorry about that.

Oh, it's my fault. I'll fix right now. Thank you again.

* Implement both null and type checking in equals functions. * Fix wrong type casting logic in SimpleJavaBean2.equals. * Add `implement Cloneable` to `UTF8String` and `SortedIterator`. * Remove dereferencing before null check in `AbstractBytesToBytesMapSuite`. * Fix coding style: Add '{}' to single `for` statement in mllib examples. * Remove unused imports in `ColumnarBatch`.

dongjoon-hyun · 2016-03-08T11:44:39Z

Hi, @srowen . I've finished.
Let's see the result, Jenkins #52663

SparkQA · 2016-03-08T13:45:56Z

Test build #52662 has finished for PR 11530 at commit cdb6077.

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

SparkQA · 2016-03-08T13:59:18Z

Test build #52663 has finished for PR 11530 at commit e3c4cf0.

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

dongjoon-hyun · 2016-03-08T17:55:15Z

Hi, @srowen .
I'm back online now. Please let me know if there is more things to do here.
Thank you for reviewing and guiding in these days.

srowen · 2016-03-09T10:12:51Z

Merged to master

dongjoon-hyun · 2016-03-09T16:27:43Z

Thank you, @srowen !

## What changes were proposed in this pull request? This issue fixes the following potential bugs and Java coding style detected by Coverity and Checkstyle. - Implement both null and type checking in equals functions. - Fix wrong type casting logic in SimpleJavaBean2.equals. - Add `implement Cloneable` to `UTF8String` and `SortedIterator`. - Remove dereferencing before null check in `AbstractBytesToBytesMapSuite`. - Fix coding style: Add '{}' to single `for` statement in mllib examples. - Remove unused imports in `ColumnarBatch` and `JavaKinesisStreamSuite`. - Remove unused fields in `ChunkFetchIntegrationSuite`. - Add `stop()` to prevent resource leak. Please note that the last two checkstyle errors exist on newly added commits after [SPARK-13583](https://issues.apache.org/jira/browse/SPARK-13583). ## How was this patch tested? manual via `./dev/lint-java` and Coverity site. Author: Dongjoon Hyun <dongjoon@apache.org> Closes apache#11530 from dongjoon-hyun/SPARK-13692.

dongjoon-hyun changed the title ~~[SPARK-13692] Fix trivial Coverity/Checkstyle defects~~ [SPARK-13692][CORE][SQL] Fix trivial Coverity/Checkstyle defects Mar 4, 2016

srowen reviewed Mar 5, 2016
View reviewed changes

srowen reviewed Mar 6, 2016
View reviewed changes

srowen reviewed Mar 8, 2016
View reviewed changes

dongjoon-hyun added 2 commits March 8, 2016 03:11

Remove unused fields and add close() to prevent resource leak.

c911f71

dongjoon-hyun added 4 commits March 8, 2016 03:11

Use stop() instead of close().

5467b90

Add a fix on JavaKinesisStreamSuit

03ac28c

Remove close() for streaming.

cdb6077

Remove streaming example

e3c4cf0

asfgit closed this in f3201ae Mar 9, 2016

dongjoon-hyun deleted the SPARK-13692 branch March 29, 2016 05:26

[SPARK-13692][CORE][SQL] Fix trivial Coverity/Checkstyle defects #11530

[SPARK-13692][CORE][SQL] Fix trivial Coverity/Checkstyle defects #11530

Conversation

dongjoon-hyun commented Mar 4, 2016

What changes were proposed in this pull request?

How was this patch tested?

dongjoon-hyun commented Mar 4, 2016

srowen Mar 5, 2016

Choose a reason for hiding this comment

dongjoon-hyun Mar 5, 2016

Choose a reason for hiding this comment

srowen Mar 6, 2016

Choose a reason for hiding this comment

srowen commented Mar 5, 2016

srowen commented Mar 5, 2016

dongjoon-hyun commented Mar 5, 2016

SparkQA commented Mar 5, 2016

dongjoon-hyun commented Mar 5, 2016

dongjoon-hyun commented Mar 5, 2016

dongjoon-hyun commented Mar 5, 2016

dongjoon-hyun commented Mar 5, 2016

dongjoon-hyun commented Mar 6, 2016

srowen Mar 6, 2016

Choose a reason for hiding this comment

dongjoon-hyun Mar 6, 2016

Choose a reason for hiding this comment

dongjoon-hyun commented Mar 6, 2016

srowen commented Mar 7, 2016

SparkQA commented Mar 7, 2016

dongjoon-hyun commented Mar 7, 2016

dongjoon-hyun commented Mar 7, 2016

dongjoon-hyun commented Mar 7, 2016

dongjoon-hyun commented Mar 7, 2016

zsxwing commented Mar 7, 2016

dongjoon-hyun commented Mar 7, 2016

SparkQA commented Mar 8, 2016

zsxwing commented Mar 8, 2016

SparkQA commented Mar 8, 2016

dongjoon-hyun commented Mar 8, 2016

dongjoon-hyun commented Mar 8, 2016

srowen Mar 8, 2016

Choose a reason for hiding this comment

dongjoon-hyun Mar 8, 2016

Choose a reason for hiding this comment

dongjoon-hyun commented Mar 8, 2016

SparkQA commented Mar 8, 2016

SparkQA commented Mar 8, 2016

dongjoon-hyun commented Mar 8, 2016

srowen commented Mar 9, 2016

dongjoon-hyun commented Mar 9, 2016