-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-9002][CORE] KryoSerializer initialization does not include 'Array[Int]' #17482
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.
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
Conversation
|
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. |
|
Jenkins add to whitelist |
| val t = Array(0, 1, 2) | ||
| assert(ser.deserialize[Array[Int]](ser.serialize(t)) === t) | ||
| } | ||
|
|
There was a problem hiding this comment.
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
|
@srowen, @BenFradet Please take a look. |
| } | ||
|
|
||
| test("basic types") { | ||
| val conf = new SparkConf(false) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
Test build #75394 has finished for PR 17482 at commit
|
BenFradet
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
|
Test build #75395 has finished for PR 17482 at commit
|
|
Test build #75400 has finished for PR 17482 at commit
|
|
@srowen Is it all right if I ask you to merge the PR? |
|
I would not ping within a few days. We leave changes open for comment for a while if it's not a rush. |
|
Thank you. |
|
Merged to master |
|
@dbolshak let me know your JIRA handle and I'll credit you at https://issues.apache.org/jira/browse/SPARK-9002 |
[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.