[AMORO-3356] Include ResourceSerde interface and impl with SimpleSerialize and KryoSerialize #3361
Conversation
…alize and KryoSerialize
|
|
||
| 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?
| 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
|
|
||
| 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.
|
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. |
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