-
Notifications
You must be signed in to change notification settings - Fork 817
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
InstantiatorStrategy inflexible #198
Comments
Would you be OK if we add these two methods?
|
Sure. The distinction between 'strategy' and 'defaultStrategy' remains unclear to me, if you aren't removing 'defaultStrategy' then can you add docs explaining its use? |
The idea was that default serializers (e.g. the ones provided by Kryo out of the box) are supposed to use DefaultStrategy under all circumstances, because it exactly models the old behavior. Any other serializers may use a custom user-defined strategy. But using user-defined strategy for Kryo's built-in serializers may result in incompatibilities with previous versions and other problems. |
I see. Turns out that 'addDefaultSerializer' is very handy for users to provide a serializer for any instanceof a type T, so defaultSerializer is now leaked to user type serialization which is unfortunate. |
Ideally we don't special case to be backward compatible, and if we do then we shouldn't make it public API. What is the drawback to removing defaultStrategy, Serializer#setDefaultSerializer, etc? I'm ok with it not being backward compatible, but I'm not clear on how it may break the default serializers with a customized strategy and on how a user would fix that. |
I need to use a fallback strategy all the time. I configure it like this
Problem is, inside Kryo we have
defaultStrategy cannot be accessed or overridden, but it's used inside newInstance(Class) and because I add my own default serializers the code path is invoked.
Can 'defaultStrategy' be removed, instead using 'strategy'? This would solve my problem and as a workaround I'll override newInstance with this logic.
The text was updated successfully, but these errors were encountered: