Adding a ScheduledDataLoaderRegistry #87
Conversation
|
cc: @tinnou - could you have a look at this please |
| public static class Builder { | ||
|
|
||
| private ScheduledExecutorService scheduledExecutorService = Executors.newSingleThreadScheduledExecutor(); | ||
| private DispatchPredicate dispatchPredicate = (key, dl) -> true; |
There was a problem hiding this comment.
what is the intent of having the dispatch predicate on the registry vs dataloader?
There was a problem hiding this comment.
I thought about it on the dataloader itself - but the more I thought about it the more it felt like a cross cutting concern
eg in graphql you dispatch the DataLoaderRegistry and that delegates to each DataLoader
Plus it would mean all the extra baggage of Executors and so on would be in each DataLoader.
This allows for a collection of DataLoaders controlled by the one mechanism.
But since the predicate gets the DL and decides on dispatch or not - you can get as finicky as you like (as if it wa inside the dataloader)
| /** | ||
| * This predicate will return true if the {@link DataLoader#dispatchDepth()} is greater than the specified depth. | ||
| * | ||
| * This will act as minimum batch size. There must be more then `depth` items queued for the predicate to return true. |
| perform that check again. Once a predicate evaluated to true, it will not reschedule and another call to | ||
| `dispatchAll` is required to be made. | ||
|
|
||
| This allows you to do things like "dispatch ONLY if the queue depth is > 10 deep or more than 200 millis have passed |
There was a problem hiding this comment.
gotcha, it allows to dial the aggressiveness of the batching strategy.
|
Looks great thank you @bbakerman for this |
| * A predicate class used by {@link ScheduledDataLoaderRegistry} to decide whether to dispatch or not | ||
| */ | ||
| @FunctionalInterface | ||
| public interface DispatchPredicate { |
There was a problem hiding this comment.
This is very similar to Predicate, why not use that?
There was a problem hiding this comment.
because it takes 2 arguments - so inspired by but not the same
|
I am not so sure about this PR and the relationship to The subclassing works, but I would argue it violates Liskov by changing the semantics of |
…r-registry # Conflicts: # README.md
A DataLoaderRegistry that will evaluate a predicate before actually calling dispatch on the dataloaders within
if they return false, then it reschedules the dispatch call for another time (schedule duration) and trys again.
This PR was inspired by the work in #77 - the difference is the threading is outside the DL classes and it has a flexible predicate on when to dispatch, allowing people to write custom logic here