Handling serialization of Default Proto instance #565
Conversation
|
|
nikhilsu
force-pushed
the
nikhilsu/fix-proto-ser-edges
branch
from
0b3a6c3 to
e0c60d6
Compare
|
@regadas thanks for the review. Just ran |
| @@ -69,6 +69,9 @@ public void write(Kryo kryo, Output output, Message mes) { | |||
| public Message read(Kryo kryo, Input input, Class<Message> pbClass) { | |||
There was a problem hiding this comment.
@regadas Wanted to get your thoughts on handling null in the serializer.
I was thinking maybe we could serialize nulls to a magic number (-1) which would look like:
@Override
public void write(Kryo kryo, Output output, Message mes) {
if (mes == null) {
output.writeInt(-1, true);
}
...
}and
@Override
public Message read(Kryo kryo, Input input, Class<Message> pbClass) {
try {
int size = input.readInt(true);
if (size == -1) {
return null;
}
...
} catch (Exception e) {
throw new RuntimeException("Could not create " + pbClass, e);
}
}
There was a problem hiding this comment.
since kryo dispatches serialization based on the .getClass I think this code will never really be called on null in practice.
Can you show a test that fails now just using Kyro not the serializer directly?
There was a problem hiding this comment.
@johnynek you're right, I cannot get the test to fail inside the serializer with null. However, the com.esotericsoftware.kryo.Serializer base class has a acceptsNull field that's injected through the constructor. Hence, depending on how it's initialized, the serializer might get a null object?
| @@ -69,6 +69,9 @@ public void write(Kryo kryo, Output output, Message mes) { | |||
| public Message read(Kryo kryo, Input input, Class<Message> pbClass) { | |||
| try { | |||
| int size = input.readInt(true); | |||
| if (size == 0) { | |||
| return (Message) pbClass.getMethod("getDefaultInstance").invoke(null); | |||
There was a problem hiding this comment.
do we actually need to use reflection here?
Should we cache the method in private variable so we don't need to make this call on each default value?
There was a problem hiding this comment.
- Yes, we do as the
getDefaultInstancemethod, like theparseFrommethod is not defined on theMessageabstraction. - Good point. Might be better to cache it as well.
There was a problem hiding this comment.
Good point on caching! reflection is expensive.
One thing about though is that this inability to parseFrom empty byte array only happens with old versions of protoc. I believe the proposed check is still useful in case somebody uses older versions.
Here we use a very old version so #571 updates it.
Codecov Report
@@ Coverage Diff @@
## develop #565 +/- ##
========================================
Coverage 91.69% 91.69%
========================================
Files 42 42
Lines 1397 1397
Branches 36 32 -4
========================================
Hits 1281 1281
Misses 116 116 Continue to review full report at Codecov.
|
|
looks like the test fails. Does it pass for you locally? |
@johnynek sorry the tests caught a silly mistake I'd made. They should pass now - ran them locally. (Red-Green-Refactor for the win!) |
| @@ -43,21 +43,30 @@ | |||
| * classes in play, which should not be very large. | |||
| * We can replace with a LRU if we start to see any issues. | |||
| */ | |||
| final protected HashMap<Class, Method> methodCache = new HashMap<Class, Method>(); | |||
| final protected HashMap<Class, Method> parseMethodCache = new HashMap<Class, Method>(); | |||
There was a problem hiding this comment.
can we keep the name methodCache to not change binary compatibility? We can just comment that this is the parseMethod?
There was a problem hiding this comment.
@johnynek Good point. Done!
| } | ||
| return meth; | ||
| } | ||
|
|
||
| protected Method getParse(Class cls) throws Exception { | ||
| return getMethodFromCache(cls, methodCache, "parseFrom", new Class[]{byte[].class}); |
There was a problem hiding this comment.
one thing I just noticed: we unconditionally allocate this every call now, in the past we only hit if once (when the cache was empty).
Can we make a private variable to hold this and allocate it only one time?
There was a problem hiding this comment.
hmm, I could also do:
return getMethodFromCache(cls, methodCache, "parseFrom", byte[].class);
|
Thank you! |
|
Thank you @johnynek! Do we know when the next release would be? This fixes an issue we are facing in our Flink Job. |
|
I think we will do one really soon! probably after #571 |
|
Oh cool. Will the release be announced somewhere? I'd want to be notified. |
|
New release is out https://github.com/twitter/chill/releases/tag/v0.10.0 |
|
Awesome! Thanks for the update. |
|
Thanks @regadas ! |
|
Thank you! no problem 👍 |
A Default instance of a Protobuf class serializes to an empty byte array. Hence, an exception is raised when trying to read from a stream with length = 0. Example stacktrace(from Red-Green TDD):