Skip to content

[SPARK-5949] HighlyCompressedMapStatus needs more classes registered w/ kryo #4877

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
Closed
squito wants to merge 7 commits into from
Closed

[SPARK-5949] HighlyCompressedMapStatus needs more classes registered w/ kryo #4877

squito wants to merge 7 commits into from

Conversation

@squito
Copy link
Contributor

@squito squito commented Mar 3, 2015

@SparkQA
Copy link

SparkQA commented Mar 3, 2015

Test build #28235 has started for PR 4877 at commit 97beaf8.

  • This patch merges cleanly.

rxin
rxin reviewed Mar 3, 2015
View reviewed changes
core/src/main/scala/org/apache/spark/serializer/KryoSerializer.scala Outdated
Copy link
Contributor

@rxin rxin Mar 3, 2015

Choose a reason for hiding this comment

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

can we move this right below array[byte]?

@rxin
Copy link
Contributor

rxin commented Mar 3, 2015

lgtm otherwise

@SparkQA
Copy link

SparkQA commented Mar 3, 2015

Test build #28235 has finished for PR 4877 at commit 97beaf8.

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

@AmplabJenkins
Copy link

AmplabJenkins commented Mar 3, 2015

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

@SparkQA
Copy link

SparkQA commented Mar 3, 2015

Test build #28239 has started for PR 4877 at commit f9a0b7c.

  • This patch merges cleanly.

vanzin
vanzin reviewed Mar 3, 2015
View reviewed changes
core/src/test/scala/org/apache/spark/serializer/KryoSerializerSuite.scala Outdated
Copy link
Contributor

@vanzin vanzin Mar 3, 2015

Choose a reason for hiding this comment

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

Would this actually fail without registration? My understanding is that the serialized data would contain class names instead of a more efficient identifier, but would still work.

Copy link
Contributor

@rxin rxin Mar 3, 2015

Choose a reason for hiding this comment

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

He set "spark.kryo.registrationRequired" to true

Copy link
Contributor

@vanzin vanzin Mar 3, 2015

Choose a reason for hiding this comment

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

Ah, I see. Thanks for pointing that out.

@SparkQA
Copy link

SparkQA commented Mar 3, 2015

Test build #28240 has started for PR 4877 at commit 09610c6.

  • This patch merges cleanly.

rxin
rxin reviewed Mar 3, 2015
View reviewed changes
core/src/test/scala/org/apache/spark/serializer/KryoSerializerSuite.scala Outdated
Copy link
Contributor

@rxin rxin Mar 3, 2015

Choose a reason for hiding this comment

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

@squito For this one I think it is fine to merge, but for future ones, this line should go above all other subpackages. We do have some violations of them in the codebase, but that's the general guideline.

Copy link
Contributor Author

@squito squito Mar 3, 2015

Choose a reason for hiding this comment

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

ah, I see. after getting it wrong elsewhere I tried relying on IntelliJ to do the ordering for me, but it does it this way. Does Aaron's plugin handle this correctly? otherwise I'm gonna really need to work hard to get in this habit :P

Copy link
Contributor Author

@squito squito Mar 3, 2015

Choose a reason for hiding this comment

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

The plugin does get it right! thanks @aarondav

@SparkQA
Copy link

SparkQA commented Mar 3, 2015

Test build #28241 has started for PR 4877 at commit a5cb744.

  • This patch merges cleanly.

@squito
Copy link
Contributor Author

squito commented Mar 3, 2015

ok I think I fixed the style issues

rxin
rxin reviewed Mar 3, 2015
View reviewed changes
core/src/test/scala/org/apache/spark/serializer/KryoSerializerSuite.scala Outdated
Copy link
Contributor

@rxin rxin Mar 3, 2015

Choose a reason for hiding this comment

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

good find on the dense vs sparse thing.

can you add a space before { ?

@SparkQA
Copy link

SparkQA commented Mar 3, 2015

Test build #28242 has started for PR 4877 at commit 5f6bb6d.

  • This patch merges cleanly.

@rxin
Copy link
Contributor

rxin commented Mar 3, 2015

i'd really like to automate the style checker so it can catch more stuff like this ... :)

@SparkQA
Copy link

SparkQA commented Mar 3, 2015

Test build #28240 has finished for PR 4877 at commit 09610c6.

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

@AmplabJenkins
Copy link

AmplabJenkins commented Mar 3, 2015

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

@SparkQA
Copy link

SparkQA commented Mar 3, 2015

Test build #28243 has started for PR 4877 at commit 7e13316.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Mar 3, 2015

Test build #28239 has finished for PR 4877 at commit f9a0b7c.

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

@AmplabJenkins
Copy link

AmplabJenkins commented Mar 3, 2015

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

@SparkQA
Copy link

SparkQA commented Mar 3, 2015

Test build #28241 has finished for PR 4877 at commit a5cb744.

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

@AmplabJenkins
Copy link

AmplabJenkins commented Mar 3, 2015

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

@SparkQA
Copy link

SparkQA commented Mar 3, 2015

Test build #28242 has finished for PR 4877 at commit 5f6bb6d.

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

@AmplabJenkins
Copy link

AmplabJenkins commented Mar 3, 2015

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

@SparkQA
Copy link

SparkQA commented Mar 3, 2015

Test build #28243 has finished for PR 4877 at commit 7e13316.

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

@AmplabJenkins
Copy link

AmplabJenkins commented Mar 3, 2015

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

@rxin
Copy link
Contributor

rxin commented Mar 3, 2015

Thanks. Merging in.

asfgit pushed a commit that referenced this pull request Mar 3, 2015
@squito @rxin
[SPARK-5949] HighlyCompressedMapStatus needs more classes registered …
9a0b75c 
…w/ kryo

https://issues.apache.org/jira/browse/SPARK-5949

Author: Imran Rashid <irashid@cloudera.com>

Closes #4877 from squito/SPARK-5949_register_roaring_bitmap and squashes the following commits:

7e13316 [Imran Rashid] style style style
5f6bb6d [Imran Rashid] more style
709bfe0 [Imran Rashid] style
a5cb744 [Imran Rashid] update tests to cover both types of RoaringBitmapContainers
09610c6 [Imran Rashid] formatting
f9a0b7c [Imran Rashid] put primitive array registrations together
97beaf8 [Imran Rashid] SPARK-5949 HighlyCompressedMapStatus needs more classes registered w/ kryo

(cherry picked from commit 1f1fccc)
Signed-off-by: Reynold Xin <rxin@databricks.com>
@asfgit asfgit closed this in 1f1fccc Mar 3, 2015
@squito squito deleted the SPARK-5949_register_roaring_bitmap branch April 4, 2015 00:41
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

@squito @SparkQA @rxin @AmplabJenkins @vanzin