Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

SPARK-3926 [CORE] Reopened: result of JavaRDD collectAsMap() is not serializable #3587

Closed
wants to merge 2 commits into from
Closed

SPARK-3926 [CORE] Reopened: result of JavaRDD collectAsMap() is not serializable #3587

wants to merge 2 commits into from

Conversation

srowen
Copy link
Member

@srowen srowen commented Dec 3, 2014

My original 'fix' didn't fix at all. Now, there's a unit test to check whether it works. Of the two options to really fix it -- copy the Map to a java.util.HashMap, or copy and modify Scala's implementation in Wrappers.MapWrapper, I went with the latter.

@SparkQA
Copy link

SparkQA commented Dec 3, 2014

Test build #24099 has started for PR 3587 at commit 12e4ecf.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Dec 3, 2014

Test build #24099 has finished for PR 3587 at commit 12e4ecf.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

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

@SparkQA
Copy link

SparkQA commented Dec 3, 2014

Test build #24102 has started for PR 3587 at commit 7bb0e66.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Dec 3, 2014

Test build #24102 has finished for PR 3587 at commit 7bb0e66.

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

@AmplabJenkins
Copy link

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

zsxwing
zsxwing reviewed Dec 4, 2014
View reviewed changes
core/src/main/scala/org/apache/spark/api/java/JavaUtils.scala

// Add no-arg constructor just for serialization
def this() = this(null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove this() since you have already copying the codes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It won't have a no-arg constructor in addition to its main one arg constructor otherwise right? This is the point of this change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran your unit test after removing the no-arg constructor, and it worked fine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, so it does. Maybe I misunderstood the original error. It's complaining about the superclass (MapWrapper) not having a no-arg constructor? So copying the class works, since we no longer subclass MapWrapper, but the copy in SerializableMapWrapper need not define a no-arg constructor. OK, that line can be removed.

@SparkQA
Copy link

SparkQA commented Dec 5, 2014

Test build #24184 has started for PR 3587 at commit 8586bb9.

  • This patch merges cleanly.

@zsxwing
Copy link
Member

zsxwing commented Dec 5, 2014

LGTM

@SparkQA
Copy link

SparkQA commented Dec 5, 2014

Test build #24184 has finished for PR 3587 at commit 8586bb9.

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

@AmplabJenkins
Copy link

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

@JoshRosen
Copy link
Contributor

LGTM, too. Merging into master and tagging for branch-1.2 backport.

@asfgit asfgit closed this in e829bfa Dec 9, 2014
asfgit pushed a commit that referenced this pull request Dec 9, 2014
@srowen @JoshRosen
SPARK-3926 [CORE] Reopened: result of JavaRDD collectAsMap() is not s…
fe7d7a9 
…erializable

My original 'fix' didn't fix at all. Now, there's a unit test to check whether it works. Of the two options to really fix it -- copy the `Map` to a `java.util.HashMap`, or copy and modify Scala's implementation in `Wrappers.MapWrapper`, I went with the latter.

Author: Sean Owen <sowen@cloudera.com>

Closes #3587 from srowen/SPARK-3926 and squashes the following commits:

8586bb9 [Sean Owen] Remove unneeded no-arg constructor, and add additional note about copied code in LICENSE
7bb0e66 [Sean Owen] Make SerializableMapWrapper actually serialize, and add unit test

(cherry picked from commit e829bfa)
Signed-off-by: Josh Rosen <joshrosen@databricks.com>
@JoshRosen
Copy link
Contributor

Merged to branch-1.1.

@srowen srowen deleted the SPARK-3926 branch December 9, 2014 17:15
@JoshRosen
Copy link
Contributor

I've merged this into branch-1.2.

asfgit pushed a commit that referenced this pull request Dec 17, 2014
@srowen @JoshRosen
SPARK-3926 [CORE] Reopened: result of JavaRDD collectAsMap() is not s…
26dfac6 
…erializable

My original 'fix' didn't fix at all. Now, there's a unit test to check whether it works. Of the two options to really fix it -- copy the `Map` to a `java.util.HashMap`, or copy and modify Scala's implementation in `Wrappers.MapWrapper`, I went with the latter.

Author: Sean Owen <sowen@cloudera.com>

Closes #3587 from srowen/SPARK-3926 and squashes the following commits:

8586bb9 [Sean Owen] Remove unneeded no-arg constructor, and add additional note about copied code in LICENSE
7bb0e66 [Sean Owen] Make SerializableMapWrapper actually serialize, and add unit test

(cherry picked from commit e829bfa)
Signed-off-by: Josh Rosen <joshrosen@databricks.com>
@JoshRosen JoshRosen mentioned this pull request Apr 2, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@srowen @SparkQA @AmplabJenkins @zsxwing @JoshRosen