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-7392][Core] bugfix: Kryo buffer size cannot be larger than 2M #5934

Closed
wants to merge 4 commits into from
Closed

[SPARK-7392][Core] bugfix: Kryo buffer size cannot be larger than 2M #5934

wants to merge 4 commits into from

Conversation

liyezhang556520
Copy link
Contributor

No description provided.

@srowen
Copy link
Member

srowen commented May 6, 2015

Ah good catch I think. CC @ilganeli

@SparkQA
Copy link

SparkQA commented May 6, 2015

Test build #31951 has finished for PR 5934 at commit d91e5ed.

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

@SparkQA
Copy link

SparkQA commented May 6, 2015

Test build #31963 has finished for PR 5934 at commit 9bf93e9.

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

ilganeli
ilganeli reviewed May 6, 2015
View reviewed changes
core/src/main/scala/org/apache/spark/serializer/KryoSerializer.scala
@@ -51,9 +51,9 @@ class KryoSerializer(conf: SparkConf)

private val bufferSizeKb = conf.getSizeAsKb("spark.kryoserializer.buffer", "64k")

if (bufferSizeKb >= 2048) {
if (bufferSizeKb >= 2048 * 1024) {
Copy link

Choose a reason for hiding this comment

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

You could use ByteUnit.MiB.toKiB(2) for clarity here.

@liyezhang556520
Copy link
Contributor Author

@ilganeli , Thank you for your comments, code updated.

@SparkQA
Copy link

SparkQA commented May 6, 2015

Test build #31983 has finished for PR 5934 at commit 5707e04.

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

@srowen
Copy link
Member

srowen commented May 6, 2015

Jenkins, retest this please

@SparkQA
Copy link

SparkQA commented May 6, 2015

Test build #31982 has finished for PR 5934 at commit 8693288.

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

@SparkQA
Copy link

SparkQA commented May 6, 2015

Test build #31989 has finished for PR 5934 at commit 5707e04.

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

asfgit pushed a commit that referenced this pull request May 8, 2015
@liyezhang556520 @srowen
[SPARK-7392] [CORE] bugfix: Kryo buffer size cannot be larger than 2M
f5e9678 
Author: Zhang, Liye <[email protected]>

Closes #5934 from liyezhang556520/kryoBufSize and squashes the following commits:

5707e04 [Zhang, Liye] fix import order
8693288 [Zhang, Liye] replace multiplier with ByteUnit methods
9bf93e9 [Zhang, Liye] add tests
d91e5ed [Zhang, Liye] change kb to mb

(cherry picked from commit c2f0821)
Signed-off-by: Sean Owen <[email protected]>
@asfgit asfgit closed this in c2f0821 May 8, 2015
JoshRosen
JoshRosen reviewed May 24, 2015
View reviewed changes
core/src/test/scala/org/apache/spark/serializer/KryoSerializerSuite.scala
@@ -32,6 +32,36 @@ class KryoSerializerSuite extends FunSuite with SharedSparkContext {
conf.set("spark.serializer", "org.apache.spark.serializer.KryoSerializer")
conf.set("spark.kryo.registrator", classOf[MyRegistrator].getName)

test("configuration limits") {
Copy link
Contributor

Choose a reason for hiding this comment

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

General style note: this test has a lot of repetition in it; the fact that we have four conf variables, conf1, conf2, conf3, and conf4, suggests that this could have benefitted from an inline def that abstracts away the actual configuration manipulation and KryoSerializer instantiation. Either this or whitespace between the individual cases would make this test easier to quickly scan through.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, it would have been nice to have the test name reference the JIRA number.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll make a new PR.

asfgit pushed a commit that referenced this pull request May 26, 2015
@liyezhang556520 @srowen
[SPARK-7854] [TEST] refine Kryo test suite
6309912 
this modification is according to JoshRosen 's comments, for details, please refer to [#5934](https://github.com/apache/spark/pull/5934/files#r30949751).

Author: Zhang, Liye <[email protected]>

Closes #6395 from liyezhang556520/kryoTest and squashes the following commits:

da214c8 [Zhang, Liye] refine Kryo test suite accroding to Josh's comments
jeanlyn pushed a commit to jeanlyn/spark that referenced this pull request May 28, 2015
@liyezhang556520 @jeanlyn
[SPARK-7392] [CORE] bugfix: Kryo buffer size cannot be larger than 2M
9a7e3cb 
Author: Zhang, Liye <[email protected]>

Closes apache#5934 from liyezhang556520/kryoBufSize and squashes the following commits:

5707e04 [Zhang, Liye] fix import order
8693288 [Zhang, Liye] replace multiplier with ByteUnit methods
9bf93e9 [Zhang, Liye] add tests
d91e5ed [Zhang, Liye] change kb to mb
jeanlyn pushed a commit to jeanlyn/spark that referenced this pull request Jun 12, 2015
@liyezhang556520 @jeanlyn
[SPARK-7392] [CORE] bugfix: Kryo buffer size cannot be larger than 2M
ef064fc 
Author: Zhang, Liye <[email protected]>

Closes apache#5934 from liyezhang556520/kryoBufSize and squashes the following commits:

5707e04 [Zhang, Liye] fix import order
8693288 [Zhang, Liye] replace multiplier with ByteUnit methods
9bf93e9 [Zhang, Liye] add tests
d91e5ed [Zhang, Liye] change kb to mb
jeanlyn pushed a commit to jeanlyn/spark that referenced this pull request Jun 12, 2015
@liyezhang556520 @jeanlyn
[SPARK-7854] [TEST] refine Kryo test suite
561f2b5 
this modification is according to JoshRosen 's comments, for details, please refer to [apache#5934](https://github.com/apache/spark/pull/5934/files#r30949751).

Author: Zhang, Liye <[email protected]>

Closes apache#6395 from liyezhang556520/kryoTest and squashes the following commits:

da214c8 [Zhang, Liye] refine Kryo test suite accroding to Josh's comments
nemccarthy pushed a commit to nemccarthy/spark that referenced this pull request Jun 19, 2015
@liyezhang556520 @nemccarthy
[SPARK-7392] [CORE] bugfix: Kryo buffer size cannot be larger than 2M
804c2d9 
Author: Zhang, Liye <[email protected]>

Closes apache#5934 from liyezhang556520/kryoBufSize and squashes the following commits:

5707e04 [Zhang, Liye] fix import order
8693288 [Zhang, Liye] replace multiplier with ByteUnit methods
9bf93e9 [Zhang, Liye] add tests
d91e5ed [Zhang, Liye] change kb to mb
nemccarthy pushed a commit to nemccarthy/spark that referenced this pull request Jun 19, 2015
@liyezhang556520 @nemccarthy
[SPARK-7854] [TEST] refine Kryo test suite
ab645f5 
this modification is according to JoshRosen 's comments, for details, please refer to [apache#5934](https://github.com/apache/spark/pull/5934/files#r30949751).

Author: Zhang, Liye <[email protected]>

Closes apache#6395 from liyezhang556520/kryoTest and squashes the following commits:

da214c8 [Zhang, Liye] refine Kryo test suite accroding to Josh's comments
@liyezhang556520 liyezhang556520 deleted the kryoBufSize branch August 26, 2015 01:28
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
@liyezhang556520 @srowen @SparkQA @JoshRosen @ilganeli