[AMORO-3356] Include ResourceSerde interface and impl with SimpleSerialize and KryoSerialize #3361
Conversation
…alize and KryoSerialize
klion26
left a comment
klion26
left a comment
There was a problem hiding this comment.
Thanks for the contribution, left some comments, please take a look when you're free.
|
|
||
| import java.io.Serializable; | ||
|
|
||
| public class JavaSerializer<R extends Serializable> implements ResourceSerde<R> { |
There was a problem hiding this comment.
Could we add some doc for this?
|
|
||
| public class JavaSerializer<R extends Serializable> implements ResourceSerde<R> { | ||
|
|
||
| private static final KryoSerialize kryoSerialize = new KryoSerialize<>(); |
There was a problem hiding this comment.
Is there any reason to use raw type here? can we change to KryoSerialize<Object> or KryoSerialize<R> or something else?
There was a problem hiding this comment.
It seems we use KryoSerialize to de/serialize for JavaSerializer, is this the expected behavior?
There was a problem hiding this comment.
JavaSerializer is use KryoSerialize before,but different with KryoSerialize.Java deserializeResource can be input null,but KryoSerialize can't be null
| try { | ||
| return KRYO_SERIALIZER.get().serialize(resource); | ||
| } catch (Exception e) { | ||
| throw new IllegalArgumentException("serialization error of " + resource, e); |
There was a problem hiding this comment.
Do we need to catch and rethrow the exception here?
| * | ||
| * @param resource input object | ||
| */ | ||
| byte[] serializeResource(R resource); |
There was a problem hiding this comment.
not sure do we need to throw checked exception for serialzieResource and deserializeResource here
There was a problem hiding this comment.
ResourceSerde interface same to SerializationUtil.SimpleSerializer,I think now it's consistent with the original behavior of the program
|
|
||
| final class DeserializedResource<R> { | ||
| private final R resource; | ||
| private final boolean modifiedDuringDeserialization; |
There was a problem hiding this comment.
Is modifiedDuringDeserialization this for future usage? it seems hasn't been used now.
There was a problem hiding this comment.
Not currently required, this is a reserved parameter in case some objects may change in the future. When set to true, in the future we can perform some processing on these objects that may have changes
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
2 participants
Include ResourceSerde interface and impl with SimpleSerialize and KryoSerialize
Why are the changes needed?
Close #3356.
Brief change log
How was this patch tested?
Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate
Run test locally before making a pull request
Documentation