Skip to content

Conversation

@dbolshak
Copy link

[SPARK-9002][CORE] KryoSerializer initialization does not include 'Array[Int]'

What changes were proposed in this pull request?

Array[Int] has been registered in KryoSerializer.
The following file has been changed
core/src/main/scala/org/apache/spark/serializer/KryoSerializer.scala

How was this patch tested?

First, the issue was reproduced by new unit test.
Then, the issue was fixed to pass the failed test.

@srowen
Copy link
Member

srowen commented Mar 30, 2017

Yeah I had also wondered this, why things like arrays of doubles, floats, chars, Strings, etc aren't included. I don't see why not.

@srowen
Copy link
Member

srowen commented Mar 30, 2017

Jenkins add to whitelist

val t = Array(0, 1, 2)
assert(ser.deserialize[Array[Int]](ser.serialize(t)) === t)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not add registrationRequired to the tests above (l78)?

…ray[Int]'

making 'basic types' passed with spark.kryo.registrationRequired
@dbolshak
Copy link
Author

@srowen, @BenFradet Please take a look.

}

test("basic types") {
val conf = new SparkConf(false)
Copy link
Member

Choose a reason for hiding this comment

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

You could do this to the next test too, and then, I think you'd have to register Tuple2, but that sounds like a good idea.

And then the next for Scala Map, Seq, List. Again, probably a good idea, as in the JIRA. I actually don't know why this isn't done already.

Copy link
Author

Choose a reason for hiding this comment

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

Unit test have been updated, nothing new was found.

Tuples are registered starting from line 149 in KryoSerializer.scala.

…ray[Int]'

extending list of checked classes for Kryo registration
@SparkQA
Copy link

SparkQA commented Mar 30, 2017

Test build #75394 has finished for PR 17482 at commit 972dc9b.

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

Copy link
Contributor

@BenFradet BenFradet left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@SparkQA
Copy link

SparkQA commented Mar 30, 2017

Test build #75395 has finished for PR 17482 at commit 20517a5.

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

@SparkQA
Copy link

SparkQA commented Mar 30, 2017

Test build #75400 has finished for PR 17482 at commit 89eb1b6.

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

@dbolshak
Copy link
Author

@srowen Is it all right if I ask you to merge the PR?

@srowen
Copy link
Member

srowen commented Mar 31, 2017

I would not ping within a few days. We leave changes open for comment for a while if it's not a rush.

@dbolshak
Copy link
Author

Thank you.

@srowen
Copy link
Member

srowen commented Apr 1, 2017

I wonder if @davies @zsxwing @squito know of any reason this wouldn't be a good idea? seems like an obvious thing to do but I wonder what I might be missing. Maybe it's just one of those things that was added to on a case-by-case basis as needed

@srowen
Copy link
Member

srowen commented Apr 3, 2017

Merged to master

@srowen
Copy link
Member

srowen commented Apr 3, 2017

@dbolshak let me know your JIRA handle and I'll credit you at https://issues.apache.org/jira/browse/SPARK-9002

@asfgit asfgit closed this in fb5869f Apr 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants