Breaking change - adds a name to a DataLoader #193
Conversation
…of factory methods
| @@ -70,331 +70,29 @@ | |||
| @NullMarked | |||
| public class DataLoader<K, V> { | |||
There was a problem hiding this comment.
should DataLoader be final btw?
…of factory methods - added support in DLR for namings DLs
…of factory methods - added support in DLR for namings DLs - added direct register
| private final DataLoaderHelper<K, V> helper; | ||
| private final StatisticsCollector stats; | ||
| private final CacheMap<Object, V> futureCache; | ||
| private final ValueCache<K, V> valueCache; | ||
| private final DataLoaderOptions options; | ||
| private final Object batchLoadFunction; | ||
|
|
||
| /** |
There was a problem hiding this comment.
These have been deprecated for FOREVER and since this is a breaking change chance - I am taking it
| assertState(name != null, () -> "The DataLoader must have a non null name"); | ||
| dataLoaders.put(name, nameAndInstrumentDL(name, instrumentation, dataLoader)); | ||
| return this; | ||
| } |
There was a problem hiding this comment.
New - we can register "named" DL now
| assertState(key.equals(dataLoader.getName()), | ||
| () -> String.format("Data loader name '%s' is not the same as registered key '%s'", dataLoader.getName(), key)); | ||
| return dataLoader; | ||
| } |
There was a problem hiding this comment.
It validates that a DL named "Foo" has been registered as the key "Foo" because dlr.register("Bar", fooDL) makes not sense
| */ | ||
| public <K, V> DataLoader<K, V> registerAndGet(String key, DataLoader<?, ?> dataLoader) { | ||
| dataLoaders.put(key, nameAndInstrumentDL(key, instrumentation, dataLoader)); | ||
| return getDataLoader(key); |
There was a problem hiding this comment.
New - gives back the DL immediately - it may have changed
| * <p> | ||
| * Note: Registration can change the data loader instance since it might get an {@link DataLoaderInstrumentation} applied to | ||
| * it. So the {@link DataLoader} instance your read via {@link DataLoaderRegistry#getDataLoader(String)} might not be the same | ||
| * object that was registered. | ||
| * | ||
| * @param key the key to put the data loader under | ||
| * @param dataLoader the data loader to register | ||
| * @return this registry | ||
| */ | ||
| public DataLoaderRegistry register(String key, DataLoader<?, ?> dataLoader) { |
There was a problem hiding this comment.
Since i can give a name to a dataloader and register it with a different name, or register the same DL multiple times with a different name (different options attached), what about providing a pass through cache key into the instrumentation i.e.
DataLoader loader, String dataLoaderKey in the instrumentation rather than attaching an explicit name to the loader itself. This way register is the SoT. I feel if we allow both we might introduce some slight oddities of dataLoaderKey != dataLoaderName else you fail at runtime with Data loader name '%s' is not the same as registered key '%s' If I attempt to do Factory create with name then register with a different name later.
Don't allow creation with a name, only allow association at registry time and pass through to instrumentation since keys can be only a registry thing.
There was a problem hiding this comment.
I gave some thought to this but there is no real link between a DataLoaderRegistry and a DataLoader.
So the instrumentation for a DL can be run without it coming from a DLR
So while you can register them "wrong" - so be it - it validates
This adds name to a DataLoader which we should have done from the start in retrospect
This helps Instrumentation so they can report stats in terms of "names".