DataLoader as an interface - 3.x branch #62
Conversation
bbakerman
changed the title
| /** | ||
| * Similar to {@link DataLoaderRegistry#dispatchAll()}, this calls {@link org.dataloader.DataLoader#dispatch()} on | ||
| * each of the registered {@link org.dataloader.DataLoader}s, but returns the number of dispatches. | ||
| * SThis calls {@link org.dataloader.DataLoader#dispatch()} on |
| @Internal | ||
| class DataLoaderHelper<K, V> { |
There was a problem hiding this comment.
The old Helper naming was a little confusing due to the class hierarchy, it makes much more sense now
| stats.incrementLoadCount(); | ||
|
|
||
| boolean batchingEnabled = loaderOptions.batchingEnabled(); | ||
| boolean cachingEnabled = loaderOptions.cachingEnabled(); | ||
|
|
||
| if (cachingEnabled) { |
There was a problem hiding this comment.
One thing that might be helpful to highlight here is the caching configuration flag now dictates whether values could be returned from the promise cache AND the value cache.
Since I can't think of a use case where someone would need the value cache but not the promise cache, I think we are good with having only one caching flag.
| CompletableFuture<V> load(K key, Object loadContext) { | ||
| synchronized (dataLoader) { | ||
| Object cacheKey = getCacheKey(nonNull(key)); | ||
| synchronized (this) { |
There was a problem hiding this comment.
Say we have caching and batching disabled and the batchFunction happens to have some blocking code (not ideal). Having this entire block synchronized would cause to only have at most one batchFunction call executed per data loader instance, no?
Do you see any risk of contention when say resolving large lists (in GraphQL)?
This is the initial cut of a 3.x branch that will move DataLoader to an interface.
As it stands right now it's just the start of the refactor. There is more to come.
However I thought I would put it up progressively to show my work.