SPARK-4297 [BUILD] Build warning fixes omnibus #3157

srowen · 2014-11-07T13:32:12Z

There are a number of warnings generated in a normal, successful build right now. They're mostly Java unchecked cast warnings, which can be suppressed. But there's a grab bag of other Scala language warnings and so on that can all be easily fixed. The forthcoming PR fixes about 90% of the build warnings I see now.

SparkQA · 2014-11-07T13:39:56Z

Test build #23054 has started for PR 3157 at commit 17bc581.

This patch merges cleanly.

SparkQA · 2014-11-07T13:41:03Z

Test build #23054 has finished for PR 3157 at commit 17bc581.

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

AmplabJenkins · 2014-11-07T13:41:04Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23054/
Test FAILed.

SparkQA · 2014-11-07T13:47:38Z

Test build #23055 has started for PR 3157 at commit 27800f7.

This patch merges cleanly.

SparkQA · 2014-11-07T15:36:11Z

Test build #23055 has finished for PR 3157 at commit 27800f7.

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

AmplabJenkins · 2014-11-07T15:36:15Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23055/
Test PASSed.

tdas · 2014-11-14T21:10:15Z

examples/src/main/scala/org/apache/spark/examples/mllib/StreamingKMeans.scala

@@ -63,7 +62,7 @@ object StreamingKMeans {
    val trainingData = ssc.textFileStream(args(0)).map(Vectors.parse)
    val testData = ssc.textFileStream(args(1)).map(LabeledPoint.parse)

-    val model = new StreamingKMeans()


Why was this change needed?

This was fixed in [https://github.com//pull/3568] by renaming the example. My apologies for not seeing this PR before submitting 3568!

Got it, I will undo this bit, and rebase too for good measure.

srowen · 2014-11-15T13:14:53Z

The object and the MLlib class have the same name so one was shadowing the other. It was correct but generated a warning IIRC. This just disambiguates.

SparkQA · 2014-11-21T17:07:50Z

Test build #23727 has started for PR 3157 at commit f4b6c5e.

This patch merges cleanly.

SparkQA · 2014-11-21T18:57:32Z

Test build #23727 has finished for PR 3157 at commit f4b6c5e.

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

AmplabJenkins · 2014-11-21T18:57:36Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23727/
Test PASSed.

SparkQA · 2014-12-03T20:25:11Z

Test build #24100 has started for PR 3157 at commit 36a5947.

This patch merges cleanly.

SparkQA · 2014-12-03T22:05:27Z

Test build #24100 has finished for PR 3157 at commit 36a5947.

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

AmplabJenkins · 2014-12-03T22:05:30Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24100/
Test PASSed.

jkbradley · 2014-12-03T22:37:40Z

@srowen With my settings, comparing warnings between master and this PR merged with master shows this PR eliminates 2 warnings (TaskResultGetter, ParquetTypes). The others don't appear (for my settings). Hopefully someone else can check too.

srowen · 2014-12-03T23:43:55Z

@jkbradley No there should be many more. Here's what I see on master now: https://gist.github.com/srowen/ddf5e606ba9cb888999f

Not all of these are addressed in the PR and some are spurious. But maybe the difference is that several of the warnings are from the build (i.e. antrun plugin), test code, and Java code?

jkbradley · 2014-12-04T03:57:40Z

@srowen I hadn't checked the test build. With that, I think I've verified all the warning fixes/suppressions expected, except for JavaRowSuite (for which I don't see any warnings in master or your PR). Hopefully someone else can check, but FWIW, this LGTM

SparkQA · 2014-12-11T19:55:37Z

Test build #24376 has started for PR 3157 at commit b5df4e7.

This patch merges cleanly.

srowen · 2014-12-11T20:02:46Z

@jkbradley I rebased just now. Hm, JavaRowSuite shows a warning in IntelliJ for an unchecked generic array creation, and I see this if I compile without the annotation:

[warn] Note: /Users/srowen/Documents/spark/sql/core/src/test/java/org/apache/spark/sql/api/java/JavaRowSuite.java uses unchecked or unsafe operations.
[warn] Note: Recompile with -Xlint:unchecked for details.

I'm going to fix a few more new, easy warnings and push. Take a look in a minute.

SparkQA · 2014-12-11T20:20:17Z

Test build #24377 has started for PR 3157 at commit 4d2847b.

This patch merges cleanly.

SparkQA · 2014-12-11T21:42:47Z

Test build #24376 has finished for PR 3157 at commit b5df4e7.

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

AmplabJenkins · 2014-12-11T21:42:51Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24376/
Test PASSed.

SparkQA · 2014-12-11T22:12:32Z

Test build #24377 has finished for PR 3157 at commit 4d2847b.

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

AmplabJenkins · 2014-12-11T22:12:36Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24377/
Test PASSed.

nchammas · 2014-12-13T06:04:37Z

+1 on this kind of cleanup work.

jkbradley · 2014-12-16T00:12:23Z

Sorry for the slow response; testing now, but it will take a bit longer to finish

SparkQA · 2014-12-19T11:57:40Z

Test build #24642 has started for PR 3157 at commit 8c9e469.

This patch merges cleanly.

srowen · 2014-12-19T11:58:35Z

Sorry for the noise on this one. I couldn't figure out why the test was failing after a rebase, but I figured it out. I think that, in the process of fixing the warning, it actually fixed the test, and, reveals a small problem in the test.

In https://github.com/apache/spark/blob/master/sql/core/src/test/scala/org/apache/spark/sql/parquet/ParquetQuerySuite.scala#L487 it looks like it should really be

    checkFilter[Operators.NotEq[Integer]](!('a.int === 1))`

instead of

    checkFilter[Operators.Not](!('a.int === 1))

@liancheng does that make sense to you? I think this was the commit with the test:
423baea

SparkQA · 2014-12-19T13:38:14Z

Test build #24642 has finished for PR 3157 at commit 8c9e469.

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

AmplabJenkins · 2014-12-19T13:38:18Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24642/
Test FAILed.

srowen · 2014-12-19T15:59:33Z

Hm, I can't see how the Hive test failure here is related to this PR. It doesn't occur for me locally and I can't see any relevant change that would cause a NoSuchMethodError. For the moment I think this is an unrelated error. That said, I don't see why other PRs aren't failing, except that perhaps they do not trigger Hive-related tests?

liancheng · 2014-12-21T13:30:16Z

@srowen Sorry for the late reply, missed this thread. Yes, the Operators.Not should be replaced with Operators.NotEq. The original Parquet filter test cases in ParquetQuerySuite didn't catch this error because the type information in this assertion is actually erased at runtime.

On the other hand, ParquetQuerySuite will be removed soon. It has been deprecated by a new set of Parquet test suites introduced in #3644. Similar type erasure problem doesn't exist in the new ParquetFilterSuite.

liancheng · 2014-12-21T13:43:05Z

Oh I see you fixed the type erasure issue, thanks :) I'm looking into the Hive test failures.

liancheng · 2014-12-21T15:27:21Z

The NoSuchMethodError was caused by wrong datanucleus-core version. The target method is defined in datanucleus-core 3.2.2 but not in 3.2.10. The former is used when compiling against Hive 0.12.0, while the latter is used when compiling against Hive 0.13.1. Currently on Jenkins we first do a clean compile against 0.12.0, then build the assembly jar without clean against 0.13.1 and run the tests.

This behavior left both versions of datanucleus-core in the lib_managed directory, and may sometimes mess up class paths. I'll open a PR to clean lib_managed before compiling Hive 0.13.1 in dev/run-tests.

I've seen this error once several days before right after the most recent Jenkins upgrade. Not sure why this issue wasn't detected before.

liancheng · 2014-12-21T15:28:52Z

Just ran dev/run-tests locally, confirmed that both sets of datanucleus jars can be found:

$ find . -name "datanucleus-*"
./lib_managed/jars/datanucleus-api-jdo-3.2.1.jar
./lib_managed/jars/datanucleus-api-jdo-3.2.6.jar
./lib_managed/jars/datanucleus-core-3.2.10.jar
./lib_managed/jars/datanucleus-core-3.2.2.jar
./lib_managed/jars/datanucleus-rdbms-3.2.1.jar
./lib_managed/jars/datanucleus-rdbms-3.2.9.jar

srowen · 2014-12-21T15:32:00Z

@liancheng Ah, good catch. Yeah sounds like a separate issue. I think this PR is good to go then, if that was the only error.

liancheng · 2014-12-21T16:40:26Z

Opened #3756 to fix the datanucleus issue.

This PR tries to fix the Hive tests failure encountered in PR #3157 by cleaning `lib_managed` before building assembly jar against Hive 0.13.1 in `dev/run-tests`. Otherwise two sets of datanucleus jars would be left in `lib_managed` and may mess up class paths while executing Hive test suites. Please refer to [this thread] [1] for details. A clean build would be even safer, but we only clean `lib_managed` here to save build time. This PR also takes the chance to clean up some minor typos and formatting issues in the comments. [1]: #3157 (comment)  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/apache/spark/3756)  Author: Cheng Lian <lian@databricks.com> Closes #3756 from liancheng/clean-lib-managed and squashes the following commits: e2bd21d [Cheng Lian] Adds lib_managed to clean set c9f2f3e [Cheng Lian] Cleans lib_managed before compiling with Hive 0.13.1 (cherry picked from commit 395b771) Signed-off-by: Josh Rosen <joshrosen@databricks.com>

This PR tries to fix the Hive tests failure encountered in PR #3157 by cleaning `lib_managed` before building assembly jar against Hive 0.13.1 in `dev/run-tests`. Otherwise two sets of datanucleus jars would be left in `lib_managed` and may mess up class paths while executing Hive test suites. Please refer to [this thread] [1] for details. A clean build would be even safer, but we only clean `lib_managed` here to save build time. This PR also takes the chance to clean up some minor typos and formatting issues in the comments. [1]: #3157 (comment)  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/apache/spark/3756)  Author: Cheng Lian <lian@databricks.com> Closes #3756 from liancheng/clean-lib-managed and squashes the following commits: e2bd21d [Cheng Lian] Adds lib_managed to clean set c9f2f3e [Cheng Lian] Cleans lib_managed before compiling with Hive 0.13.1

srowen · 2014-12-23T22:02:08Z

Jenkins, retest this please.

SparkQA · 2014-12-23T22:07:32Z

Test build #24747 has started for PR 3157 at commit 8c9e469.

This patch merges cleanly.

SparkQA · 2014-12-23T23:57:39Z

Test build #24747 has finished for PR 3157 at commit 8c9e469.

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

AmplabJenkins · 2014-12-23T23:57:43Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24747/
Test PASSed.

JoshRosen · 2014-12-24T00:07:20Z

Yay, the tests passed! @srowen it looks like I could merge this now, but are there any other updates / changes that you want to make? Also, should this be pulled into any branches other than master?

srowen · 2014-12-24T00:08:29Z

@JoshRosen No, probably enough for now. I'll have another look at cleaning up warnings in a couple months.

JoshRosen · 2014-12-24T00:15:12Z

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

@@ -1012,7 +1012,7 @@ public void testPairToPairFlatMapWithChangingTypes() { // Maps pair -> pair
          }
        });
    JavaTestUtils.attachTestOutputStream(flatMapped);
-    List<List<Tuple2<String, Integer>>> result = JavaTestUtils.runStreams(ssc, 2, 2);
+    List<List<Tuple2<Integer, String>>> result = JavaTestUtils.runStreams(ssc, 2, 2);


Wait, how did this even compile before if you were able to swap the type parameters to Tuple2?

The generic types are wrong, but the underlying objects are fine. JavaTestUtils.runStreams returns a List<List<V>> so happily binds the Tuple2 type to whatever the caller says. The reason the comparison compiled was that assertEquals(Object, Object) accepts anything. The ultimate List.equals() method doesn't care about types and compares values which are in fact correct and of the right type and equal.

Ah, that makes sense.

JoshRosen · 2014-12-24T21:32:25Z

This looks good to me. Since @liancheng and @jkbradley have done a pretty extensive pass through the changes, too, I'm going to merge this into master.

srowen mentioned this pull request Nov 12, 2014

[SPARK-4364][Streaming] Fix the variable types #3227

Closed

tdas reviewed Nov 14, 2014
View reviewed changes

srowen force-pushed the SPARK-4297 branch from 27800f7 to f4b6c5e Compare November 21, 2014 17:02

srowen force-pushed the SPARK-4297 branch from f4b6c5e to 36a5947 Compare December 3, 2014 20:20

srowen force-pushed the SPARK-4297 branch from 36a5947 to b5df4e7 Compare December 11, 2014 19:49

Suppress unchecked cast warnings, and several other build warning fixes

8c9e469

srowen force-pushed the SPARK-4297 branch from 2e3f9a6 to 8c9e469 Compare December 19, 2014 11:55

liancheng mentioned this pull request Dec 21, 2014

[SPARK-4914][Build] Cleans lib_managed before compiling with Hive 0.13.1 #3756

Closed

JoshRosen reviewed Dec 24, 2014
View reviewed changes

asfgit closed this in 29fabb1 Dec 24, 2014

srowen deleted the SPARK-4297 branch December 25, 2014 09:11

SPARK-4297 [BUILD] Build warning fixes omnibus #3157

SPARK-4297 [BUILD] Build warning fixes omnibus #3157

Conversation

srowen commented Nov 7, 2014

SparkQA commented Nov 7, 2014

SparkQA commented Nov 7, 2014

AmplabJenkins commented Nov 7, 2014

SparkQA commented Nov 7, 2014

SparkQA commented Nov 7, 2014

AmplabJenkins commented Nov 7, 2014

tdas Nov 14, 2014

Choose a reason for hiding this comment

jkbradley Dec 3, 2014

Choose a reason for hiding this comment

srowen Dec 3, 2014

Choose a reason for hiding this comment

srowen commented Nov 15, 2014

SparkQA commented Nov 21, 2014

SparkQA commented Nov 21, 2014

AmplabJenkins commented Nov 21, 2014

SparkQA commented Dec 3, 2014

SparkQA commented Dec 3, 2014

AmplabJenkins commented Dec 3, 2014

jkbradley commented Dec 3, 2014

srowen commented Dec 3, 2014

jkbradley commented Dec 4, 2014

SparkQA commented Dec 11, 2014

srowen commented Dec 11, 2014

SparkQA commented Dec 11, 2014

SparkQA commented Dec 11, 2014

AmplabJenkins commented Dec 11, 2014

SparkQA commented Dec 11, 2014

AmplabJenkins commented Dec 11, 2014

nchammas commented Dec 13, 2014

jkbradley commented Dec 16, 2014

SparkQA commented Dec 19, 2014

srowen commented Dec 19, 2014

SparkQA commented Dec 19, 2014

AmplabJenkins commented Dec 19, 2014

srowen commented Dec 19, 2014

liancheng commented Dec 21, 2014

liancheng commented Dec 21, 2014

liancheng commented Dec 21, 2014

liancheng commented Dec 21, 2014

srowen commented Dec 21, 2014

liancheng commented Dec 21, 2014

srowen commented Dec 23, 2014

SparkQA commented Dec 23, 2014

SparkQA commented Dec 23, 2014

AmplabJenkins commented Dec 23, 2014

JoshRosen commented Dec 24, 2014

srowen commented Dec 24, 2014

JoshRosen Dec 24, 2014

Choose a reason for hiding this comment

srowen Dec 24, 2014

Choose a reason for hiding this comment

JoshRosen Dec 24, 2014

Choose a reason for hiding this comment

JoshRosen commented Dec 24, 2014