Add Kryo.setClassResolver(ClassResolver) #182
Conversation
You're right, I'll add this to javadoc.
Sounds good, I can add a flag to setClassResolver that triggers copying of all registrations (if false, only registrations for primitives are copied). |
All existing registrations are copied from the previous class resolver to the new one if copyAllRegistrations is `true`, otherwise only registrations for primitive types are copied. The kryo instance is set on the new class resolver, so that the client does not have to invoke this. Also setters for ReferenceResolver and StreamFactory are adjusted to set kryo on the instances, the Kryo constructor now just uses these setters.
|
@romix I updated the commit with the proposed changes. |
| @@ -969,6 +961,41 @@ public ClassResolver getClassResolver () { | |||
| return classResolver; | |||
| } | |||
|
|
|||
| /** | |||
| * Sets the new class resolver and may copy all registrations from the former class resolver. Is expected to be invoked before | |||
| * any serialization/deserialization is performed (other behaviour is not tested). | |||
There was a problem hiding this comment.
Should throw illegal state exception if the method is called at the wrong time.
There was a problem hiding this comment.
How do I detect this?
There was a problem hiding this comment.
I'm not sure, but it's strange to allow if the behavior is undefined. If we don't copy I guess it's moot.
|
I'm not sure about the general usefulness of allowing the class resolver to change. Why do you want this? Is it app specific? Would it make more sense to do it differently? Eg create your Kryo instance with a class resolver that delegates to another class resolver. |
|
Currently specifying a ClassResolver in the constructor is mostly about which one to use, not how it is configured. If we'll have setClassResolver I think it should just be a setter for the most part and not try to copy registrations. Copying can happen outside of Kryo. I'm not sure it would be useful, but a ClassResolver may not need to keep a list of registrations, or whatever list it has may not be useful for copying. Eg, maybe the ClassResolver is doing network lookups. So let's not have ClassResolver#getRegistrations(), though implementations may have it to facilitate copying outside of setClassResovler. |
In memcached-session-manager (still using kryo1, I want to move to kryo2) it's possible to customize Kryo via a KryoCustomization, implementations can be specified in tomcats context.xml via an attribute A case where I need to customize the ClassResolver is the CGLibProxySerializer, see this CGLibProxySerializerTest. |
|
Maybe it's cleaner to pass around an instance of your own class that collects configuration information, then use that to construct the Kryo instance? Otherwise, I think a setClassResolver would be ok if its just a (relatively) simple setter. |
Yes, probably this way is cleaner, I start to like that idea, will think a bit more about this. Thanks! |
|
@magro Speaking of passing around an instance which collects configuration information and then uses it to construct Kryo instances: How about something like https://github.com/twitter/chill/blob/develop/chill-java/src/main/java/com/twitter/chill/KryoInstantiator.java provided by Chill? Also have a look at other classes in the same directory, e.g. IKryoRegistrar and its implementations. Is it what you have in mind? |
|
@romix Probably I'd use a builder to configure kryo.
|
|
I've implemented a builder based approach in msm, therefore obsolete. |
Add the possibility to set a
ClassResolverafter Kryo was constructed, but it's expectedthat
Kryo.setClassResolveris invoked before any (de)serialization is performed (other behaviouris just not tested).
The Kryo instance is set on the new class resolver, so that the client does not have
to invoke this (also setters for ReferenceResolver and StreamFactory are adjusted to set Kryo).