Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a hook to intercept registration of all new serializers. #947

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chrisr3
Copy link
Contributor

@chrisr3 chrisr3 commented Mar 24, 2023

Create a mechanism for customising, configuring, or even replacing a serializer when it is registered.

See #943.

@theigl
Copy link
Collaborator

theigl commented Mar 28, 2023

Hi @chrisr3. Do you need this change for your custom serializer registration to work, or is this just something that would be nice to have? Would overriding register be enough in your case?

@chrisr3
Copy link
Contributor Author

chrisr3 commented Mar 28, 2023

Hi @chrisr3. Do you need this change for your custom serializer registration to work, or is this just something that would be nice to have? Would overriding register be enough in your case?

Something like this would simplify my integration. The problem with overriding Kryo.register(Registration) is that not everything uses it:

public Registration register (Class type, Serializer serializer) {
	Registration registration = classResolver.getRegistration(type);
	if (registration != null) {
		registration.setSerializer(serializer);
		return registration;
	}
	return classResolver.register(new Registration(type, serializer, getNextRegistrationId()));
}

And not forgetting

public Registration register (Class type)

which invokes register(Class, Serializer).

@theigl
Copy link
Collaborator

theigl commented Mar 28, 2023

I mean something like this:

public class MyClassResolver extends DefaultClassResolver {

    @Override
    public Registration register(Registration registration) {
        return super.register(adapt(registration));
    }

    private Registration adapt(Registration registration) {
        // modify registration here
        return registration;
    }   
}

WDYT?

@chrisr3
Copy link
Contributor Author

chrisr3 commented Mar 28, 2023

WDYT?

I think my customised Kryo class must now be paired with a customised ClassResolver, and Bad Things will happen if they are separated.

And you presumably mean?

protected Registration adapt(Registration registration)

@theigl
Copy link
Collaborator

theigl commented Mar 28, 2023

Do you have full control over the Kryo instance? Or can it be changed by clients/users? If you have full control, then using a custom class resolver isn't really an issue. It's just like any other configurable part of Kryo.

@chrisr3
Copy link
Contributor Author

chrisr3 commented Mar 28, 2023

We are already providing Kryo with a modified ClassResolver, for unrelated reasons and from a different code-base. Which is why I would prefer to keep all of my Kryo-related changes encapsulated within the Kryo class.


I've tried creating a

protected Registration adapt(Registration)

method, with the goal of adapting Kryo's invocations of ClassResolver.register(Registration), except this doesn't work for its invocation of ClassResolver.registerImplicit(Class) 🤦.

@theigl
Copy link
Collaborator

theigl commented Apr 3, 2023

@chrisr3:

I gave your issue more thought over the weekend:

Even if I were to merge this change, you would still need a lot of complex code to achieve your desired behavior. Including your CollectionSerializerAdapter. I'm reluctant to add any code that only a single user needs and that doesn't even elegantly solve their problem.

I see only one option that does not require you to write a lot of workarounds: Adding something like Kryo.setOptimizedCollections(false). Both CollectionSerializer and ObjectArraySerializer could check this flag and disable the optimization.

Again, this is most likely not a feature a lot of users are going to need. It adds to Kryo's public API and will have to be maintained in the future. On the other hand, it would be straightforward to implement and could even be made protected so only users who extend Kryo can set the flag.

@chrisr3
Copy link
Contributor Author

chrisr3 commented Apr 3, 2023

@chrisr3:
Even if I were to merge this change, you would still need a lot of complex code to achieve your desired behavior. Including your CollectionSerializerAdapter. I'm reluctant to add any code that only a single user needs and that doesn't even elegantly solve their problem.

To be fair, I never intended to add my CollectionSerializerAdapter to this PR. I was simply looking to create an "officially supported" method of intercepting the creation of new serializers so that I could adapt CollectionSerializer and its sub-classes. (The ObjectArraySerializer has no sub-classes, and so I can simply override it with my own rather then adapting it.)

I see only one option that does not require you to write a lot of workarounds: Adding something like Kryo.setOptimizedCollections(false). Both CollectionSerializer and ObjectArraySerializer could check this flag and disable the optimization.

Again, this is most likely not a feature a lot of users are going to need. It adds to Kryo's public API and will have to be maintained in the future.

Yes, I agree. This is why I was only trying to create a more general extension API that my adapter could use.

@theigl
Copy link
Collaborator

theigl commented Apr 3, 2023

To be fair, I never intended to add my CollectionSerializerAdapter to this PR. I was simply looking to create an "officially supported" method of intercepting the creation of new serializers so that I could adapt CollectionSerializer and its sub-classes. (The ObjectArraySerializer has no sub-classes, and so I can simply override it with my own rather then adapting it.)

I think we misunderstood each other slightly. ;) I understand that you don't plan to add your CollectionSerializerAdapter to this PR. My main concern was that if I merge this PR, you will still have to use a lot of complex and potentially brittle (reflection) code on your side to get this working. So I was wondering if we shouldn't go the other way and make the behavior of CollectionSerializer and ObjectArraySerializer configurable.

But I just gave this a quick try and I'm not happy with the solution. Even if I add a configuration flag, CollectionSerializer still tries to find a concrete element serializer by analyzing the collections generics. So if you don't turn off generics optimization, there is at least one other code path that potentially breaks your application. So I would need to add multiple conditions to the already complex serializer.

@theigl
Copy link
Collaborator

theigl commented Apr 3, 2023

Regarding your PR:

I'm not really happy about going from Kryo to DefaultClassResolver and then calling back into Kryo. And since these classes are in different packages, this means that the method in Kryo has to be public.

Are you sure you can't simply override your class resolver directly? Or wrap it in a decorator that notifies your custom Kryo instance about new registrations?

@chrisr3
Copy link
Contributor Author

chrisr3 commented Apr 3, 2023

But I just gave this a quick try and I'm not happy with the solution. Even if I add a configuration flag, CollectionSerializer still tries to find a concrete element serializer by analyzing the collections generics. So if you don't turn off generics optimization, there is at least one other code path that potentially breaks your application. So I would need to add multiple conditions to the already complex serializer.

We're already turning the generics optimisation off because we're using Kotlin - see #864. Actually, I think I should dig deeper into that particular issue now... 🤔.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants