ISPN-1458 Allow to explicitly register a key transformer in KeyTransformationHandler

[luksa]

Sanne said this could be useful.

Allow to register a Transformer in KeyTransformationHandler (for when you can't annotate a key type with @transformable).

[Sanne]

this can't be static or we will leak classes

[Sanne]

Hi @luksa,
before we can merge this we should make sure it won't leak memory.
To solve this the KeyTransformationHandler should not be static, could you change it to create an instance in the QueryInterceptor? Then the registerTransformer could work via configuration, or how would you suggest to register them?
thank you for the patch

[Sanne]

Also make sure to open an issue on JIRA regarding these changes
https://issues.jboss.org/browse/ISPN

[luksa]

Yeah, I had a feeling the static Map would be a problem, but thought I'd just commit and you'd tell me after reviewing.

Hmm, if I make KeyTransformationHandler's methods non static, I will have to change QueryExtractorUtil, LazyIterator and a few other classes, since they all call the handler's stringToKey() method.

I could keep stringToKey() static, and instead of supplying a ClassLoader as the second parameter, clients would simply need to pass in the cache. Right now, clients are calling stringToKey like this:

KeyTransformationHandler.stringToKey(key,cache.getAdvancedCache().getClassLoader());

So all those calls would be simplified into:

KeyTransformationHandler.stringToKey(key,cache);

This would reduce duplication a little and prevent people from passing in a completely different ClassLoader.

The stringToKey() could now obtain the cache's QueryInterceptor and through it also the correct instance of the KeyTransformationHandler.

The other way to do it, would be to add a registerKeyTransformer method to SearchManager, which would delegate to QueryInterceptor, which would then delegate to KeyTransformationHandler (so I don't need to expose either QueryInterceptor or KeyTransformationHandler). Of course all the clients that are currently calling the static stringToKey() method, would now have to first obtain a SearchManager (or the QueryInterceptor directly), get the keyTransformationHandler from it and call the non-static stringToKey() method.

What do you think?

[Sanne]

Hi,
I think we should go for the non-static approach, as it paves the road for more optimisations and a cleaner code.
For example, the current getTransformer is invoked all the time (for non primitives) causing each invocation to include a reflection usage to read the annotation. that's a stupid performance issue which should be solved too.

Don't worry for the internal APIs, you can change them to do some spring cleaning if you have good ideas.

[Sanne]

@luksa sorry for the delay, I didn't get any notification of you adding the last changes. Will review soon.

[Sanne]

@luksa did you already sign the contributor agreement for Infinispan at https://cla.jboss.org ?

[luksa]

I have just signed it.

[Sanne]

thanks a lot, merged with some minor changes. The lookup service is very useful for some cases like the clustered query, in which we need it as otherwise we're without context, but generally I've moved the lookup out of the hot loops.