SPARK-5500. Document that feeding hadoopFile into a shuffle operation wi...

[sryza]

...ll cause problems

[SparkQA]

Test build #26422 has started for PR 4293 at commit 78ba008.

• This patch merges cleanly.

[SparkQA]

Test build #26422 has finished for PR 4293 at commit 78ba008.

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

[AmplabJenkins]

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

[rxin]

This is good to put. One idea that just came to my mind is ... why don't the downstream operators inspect whether they need to do copys or not?

[JoshRosen]

Even if we can't fix this problem, I wonder if we could override some of the HadoopRDD methods to log warnings when they're called (e.g. cache).

[SparkQA]

Test build #26446 has started for PR 4293 at commit 6e1932a.

• This patch merges cleanly.

[SparkQA]

Test build #26446 has finished for PR 4293 at commit 6e1932a.

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

[AmplabJenkins]

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

[sryza]

@rxin @JoshRosen I like both of those ideas. Updated patch implements Josh's . Reynold's is a little more involved, but would be good to implement down the line as well.

[rxin]

throwing an exception is a bit extreme and will break existing code. How about just a warning?

[JoshRosen]

+1; I had a recent PR that added some similar errors / warnings for common user mistakes, but I only raised exceptions for branches of the code that threw errors (e.g. NPE) 100% of the time. Let's make this into a warning.

[aarondav]

Also the error message might include the workaround; right now, it seems somewhat final.

[sryza]

Are there any conceivable situations where existing code that's doing this wouldn't have a bug?

On Jan 30, 2015, at 6:59 PM, Reynold Xin notifications@github.com wrote:

In core/src/main/scala/org/apache/spark/rdd/HadoopRDD.scala:

@@ -308,6 +309,14 @@ class HadoopRDD[K, V](
// Do nothing. Hadoop RDD should not be checkpointed.
}

• override def persist(storageLevel: StorageLevel): this.type = {
• if (storageLevel.deserialized) {

•  throw new SparkException("Can't cache HadoopRDDs as deserialized objects because Hadoop's" +
  

  throwing an exception is a bit extreme and will break existing code. How about just a warning?

—
Reply to this email directly or view it on GitHub.

[JoshRosen]

Hmm... maybe this is more akin to the "arrays shouldn't be used as keys when partitioning RDDs with HashPartitioner", which is virtually guaranteed to lead to a wrong answer.

[SparkQA]

Test build #26522 has started for PR 4293 at commit cc46e52.

• This patch merges cleanly.

[sryza]

Updated patch adds instructions on how to avoid the exception and extends behavior to NewHadoopRDD.

My opinion is still that this deserves an Exception rather than a warning. It's not a groupByKey kind of situation where there is a more performant choice - users that cache RDDs in this way face fundamental correctness issues. If I'm missing legitimate uses for this of course I take it back.

[SparkQA]

Test build #26522 has finished for PR 4293 at commit cc46e52.

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

[AmplabJenkins]

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

[sryza]

retest this please

[SparkQA]

Test build #26525 has started for PR 4293 at commit cc46e52.

• This patch merges cleanly.

[SparkQA]

Test build #26525 has finished for PR 4293 at commit cc46e52.

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

[AmplabJenkins]

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

[rxin]

remove the import here

[rxin]

@sryza the fact that we have a test case failing I think is enough of a reason to not do something too drastic here. How about just changing it to logWarning for now?

[sryza]

Looks like the test failures are legit

[SparkQA]

Test build #26538 has started for PR 4293 at commit e9ce742.

• This patch merges cleanly.

[sryza]

Ah, so the failing tests expose the possibility that someone could write their own InputFormat that doesn't reuse a single object, and caching that would be fine. So changing it to logWarning sounds fine to me.

It might also be worthwhile to suppress the warning for BinaryFilesRDD and WholeTextFileRDD, though it would look fairly weird, because persist would need to be overridden in these methods to call the grandparent class's version of the method. So I can add that if y'all prefer but will leave it out otherwise.

[SparkQA]

Test build #26538 has finished for PR 4293 at commit e9ce742.

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

[AmplabJenkins]

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

[rxin]

I'm merging this in master. Thanks Sandy.