-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
new LookupCache interface #2455
Conversation
public interface LookupCache <K,V> | ||
extends Snapshottable<LookupCache<K,V>>, | ||
java.io.Serializable { | ||
void contents(BiConsumer<K,V> consumer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to add Javadocs in general. But especially here, as to if and why iteration over entries is desired for all implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ReadOnlyClassToSerializerMap relies on this being implemented (only usage I found) - https://github.com/FasterXML/jackson-databind/blob/master/src/main/java/com/fasterxml/jackson/databind/ser/impl/ReadOnlyClassToSerializerMap.java#L36
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. Hmmh. Yes, that makes sense then... if that specific case is to be made pluggable.
Actually: let's drop that from interface, and not make this particular cache pluggable.
So contents()
is perfectly fine for SimpleLookupCache
for now. We can consider adding it into interface later on if other use is needed, but for now I'd rather keep API minimalistic.
int size(); | ||
|
||
/** | ||
* NOTE: key is of type Object only to retain binary backwards-compatibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept this from existing SimpleLookupCache documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. We wouldn't have to retain that (binary compatibility not preserved across 2.x -> 3.0 boundary) but... I don't feel strongly either way. I know that casting to type variable here won't be of much use as it's truly generic type. But it might help catch some type mismatches.
So I am fine either way at this point.
*/ | ||
public interface LookupCache <K,V> | ||
extends Snapshottable<LookupCache<K,V>>, | ||
java.io.Serializable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure java.io.Serializable
is actually needed for interface itself.
Serializability in 3.0 is by serializing Builders. Although it may be true that for some types, implementations would need to be serializable. Snapshottable does make sense, I think.
Makes sense; added some notes. |
@cowtowncoder my original intent was to support overriding the type cache in TypeFactory but the LookupCache is used in a few places (eg SerializerCache and DeserializerCache). These caches are not normally created by users directly. If we want to allow these caches to be able to use alternative cache implementations, would you have any suggestions? Maybe, we could expose something in the JsonMapper.Builder? |
UnlimitedLookupCache<Object, JavaType> cache = new UnlimitedLookupCache<>(4, 10); | ||
TypeFactory tf = TypeFactory.defaultInstance().withCache(cache); | ||
|
||
//TODO find way to inject the `tf` instance into an ObjectMapper (via MapperBuilder?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cowtowncoder any suggestions on best place to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Best place to... ? Inject cache?
Builder is the new mechanism, in general, but in case of TypeFactory
I think it is reasonable that caller would need to create and configure it directly, pass instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For other caches, however, need to think about it... they are held on to by DeserializationContext
/ SerializerProvider
, and we'd ideally avoid making developers create instances of those.
How about I'll merge this, and initially comment out |
Ok so I commented it out, but realized that... maybe that's not the way to go. May expose it back. However. I also had another idea that might nicely complement this initial part. Will add notes to main issue. |
For #2456