[#190] Fix(core): Create serde class lazily #192
Conversation
Code Coverage Report
Files
|
core/src/main/java/com/datastrato/graviton/proto/ProtoEntitySerDe.java
Outdated
Show resolved
Hide resolved
|
LGTM |
| Class<? extends Message> protoClass = | ||
| entityToProto.computeIfAbsent( | ||
| clazz, | ||
| k -> { | ||
| try { | ||
| return (Class<? extends Message>) | ||
| loadClass( | ||
| ENTITY_TO_PROTO.get(k.getCanonicalName()), getClass().getClassLoader()); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException( | ||
| "Failed to create proto class " + k.getCanonicalName(), e); | ||
| } | ||
| }); |
There was a problem hiding this comment.
I think you should make sure to put this code in a lock to avoid race conditions.
There was a problem hiding this comment.
Is there any problem with loading or putting the same class twice ?
| throw new RuntimeException( | ||
| "Failed to instantiate serde class " + entityClazz.getCanonicalName(), e); | ||
| } | ||
| }); |
|
|
||
| ProtoSerDe<T, M> protoSerDe = (ProtoSerDe<T, M>) entityToSerDe.get(entityClass); | ||
| return protoSerDe.deserialize(m); | ||
| } |
There was a problem hiding this comment.
Why do we remove this method, this method is pair with toProto, we'd better keep this.
There was a problem hiding this comment.
toProto and fromProto is only the intermediate step of serialize and deserialize at present, not called elsewhere, so I make toProto private, and remove fromProto.
There was a problem hiding this comment.
BTW, fromProto method need to add a <T extends Entity> T parameter as there may be one proto maps to multiple entity later on.
There was a problem hiding this comment.
To make pair with toProto, already add fromProto back and modified method signature
| @@ -86,52 +52,77 @@ public <T extends Entity> byte[] serialize(T t) throws IOException { | |||
| public <T extends Entity> T deserialize(byte[] bytes, Class<T> clazz, ClassLoader classLoader) | |||
| throws IOException { | |||
| Any any = Any.parseFrom(bytes); | |||
| Class<? extends Message> protoClass = | |||
| getProtoClass(clazz, Thread.currentThread().getContextClassLoader()); | |||
There was a problem hiding this comment.
I think you can use the classLoader provided in method parameter.
| private <T extends Entity, M extends Message> M toProto(T t) throws IOException { | ||
| ProtoSerDe<T, M> protoSerDe = | ||
| (ProtoSerDe<T, M>) | ||
| getProtoSerde(t.getClass(), Thread.currentThread().getContextClassLoader()); |
There was a problem hiding this comment.
I think you should take classloader as a method parameter and pass in from the caller of this method.
We'd better to make sure classloader is got only from one place and pass in to different methods, so that we could make sure all classloader is the same and change one place is enough (if we want to use different classloader).
| private <T extends Entity, M extends Message> T fromProto(M m, Class<T> entityClass) | ||
| throws IOException { | ||
| ProtoSerDe<T, Message> protoSerDe = | ||
| getProtoSerde(entityClass, Thread.currentThread().getContextClassLoader()); |
What changes were proposed in this pull request?
Lazily create the serde class in the classloader when serializing and deserializing
Why are the changes needed?
Class Not Found Exception will be thrown when serializing or deserializing catalog entity.
Fix: #190
Does this PR introduce any user-facing change?
No
How was this patch tested?
Existing UTs