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.
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.
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.
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.
I kept this from existing SimpleLookupCache documentation
There was a problem hiding this comment.
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.
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.
@cowtowncoder any suggestions on best place to do this?
There was a problem hiding this comment.
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.
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