Allow dispatch() caller to get stats back. #56
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
October 23, 2019 13:32
Allow dispatch caller to get stats back
|
There seems to be discussion of this same issue in #46 (comment) . |
bbakerman
reviewed
Feb 9, 2020
| * | ||
| * @return the promise of the queued load requests and the number of entries dispatched. | ||
| */ | ||
| public DataLoaderHelper.DispatchResult<V> dispatchWithCounts() { |
There was a problem hiding this comment.
I am going to accept this PR and fix it up but I will put the changes I will make here for you to see.
Here would would not return a nest class from a helper - I would make it a full class since it now API
bbakerman
reviewed
Feb 9, 2020
| @@ -134,7 +134,16 @@ Object getCacheKey(K key) { | |||
| loaderOptions.cacheKeyFunction().get().getKey(key) : key; | |||
| } | |||
|
|
|||
| CompletableFuture<List<V>> dispatch() { | |||
| public static class DispatchResult<X> { | |||
| public final CompletableFuture<List<X>> futureList; | |||
There was a problem hiding this comment.
I know I know Java should have data classes so we dont have to write so much code
but it does - so I would (and will) use POJO patterns with getters.
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For context: I'm working on a change to allow better composability of
DataLoaders. Right now, you can't nestDataLoader.load().thenApply()within another similar call. When a GraphQL field resolution expects a result typeV, you can return aCompletableFuture<T>(e.g. due to aDataLoader), but you can't return aCompletableFuture<CompletableFuture<T>>and so on. So, instead of being able to chainDataLoaders together, you have to come up with a specializedDataLoaderto calculate the one specific field you want. I'm trying to make it so that you can chainDataLoaders together.The bulk of the change is going to be in
graphql-java, in another PR. But that change is going to need insight into what happened when adispatchAllwas done in theDataLoaderRegistry. That's what this PR is about.Actual change: Added
dispatchWithCountswhich now returns aDispatchResultwhich includes the previously returnedCompletableFuture<List<X>>, plus a count of how many entries (keys) were dispatched. No API change to existing methods. (Aside: I did also think about changing theCompletableFuture<List<X>>to aList<CompletableFuture<X>>, which would accomplish the same thing. But that would change the API, so I opted for this approach instead.)