[Spark-27416][SQL]UnsafeMapData & UnsafeArrayData Kryo serialization breaks when two machines have different Oops size #24340
Conversation
… when two machines have different Oops size
|
cc @cloud-fan @sandeep-katta @rxin |
|
Can one of the admins verify this patch? |
| /** | ||
| * General utilities available for unsafe data | ||
| */ | ||
| public class UnsafeDataUtils { |
There was a problem hiding this comment.
Package private? and final and with private constructor for good measure
There was a problem hiding this comment.
Personally, I prefer to make this class public, as it's "UnsafeDataUtils" which may contain other public utilities. It may turn to be "package private" if it's named UnsafeSerializationUtils.
Your comments will be appreciated.
There was a problem hiding this comment.
It can be made public later if it needs to be. Otherwise it becomes something apps can inadvertently depend on in their code. We want to discourage that.
| && baseOffset == Platform.BYTE_ARRAY_OFFSET | ||
| && (((byte[]) baseObject).length == sizeInBytes)) { | ||
| return (byte[]) baseObject; | ||
| } else { |
There was a problem hiding this comment.
Not that it matters, but if you're changing this, this 'else' is redundant
There was a problem hiding this comment.
Thanks for pointing that out, feel really happy to reduct one line of code.
There was a problem hiding this comment.
It's really trivial, no need to change it. This is more just about talking about code style preferences
|
@srowen updated as discussed, please recheck when available. thanks |
srowen
left a comment
srowen
left a comment
There was a problem hiding this comment.
OK do we need to add these classes to the list that is automatically registered with Kryo by default too?
I am not quite sure what's the main advantage. If that's for performance, I think in this case it makes no difference. |
3 participants
What changes were proposed in this pull request?
Finish the rest work of #24317, #9030
a. Implement Kryo serialization for UnsafeArrayData
b. fix UnsafeMapData Java/Kryo Serialization issue when two machines have different Oops size
c. Move the duplicate code "getBytes()" to Utils.
How was this patch tested?
According Units has been added & tested