New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a helper class for making asynchronous data fetchers #798

Merged
merged 5 commits into from Oct 27, 2017

Conversation

Projects
None yet
2 participants
@RusticFlare
Contributor

RusticFlare commented Oct 25, 2017

Adds the ability to make any DataFetcher appropriate for running asynchronously via a helper class AsynchronousDataFetcher.

This class has a static method async(DataFetcher) that wraps the given Data Fetcher<T> in a new DataFetcher<CompletableFuture<T>>. The CompletableFuture returned by the new DataFetcher runs the original CompletableFuture asynchronously.

Example use (with static imports to improve readability)

GraphQLFieldDefinition.newFieldDefinition()
        .dataFetcher(async(fooDataFetcher))
        // ... rest of field definition

I have also added some stuff to the .gitignore for files generated by Eclipse.

@bbakerman

Thank for you PR. We appreciate it.

We need this to be updated to accept an ExecutorService and also reinstate the original documentation and rather add this to it as a shortcut

@Override
public CompletableFuture<T> get(DataFetchingEnvironment environment) {
return CompletableFuture.supplyAsync(() -> wrappedDataFetcher.get(environment));
}

This comment has been minimized.

@bbakerman

bbakerman Oct 25, 2017

Member

In principle I like aspects of this PR but we cant accept it as it as is because its using a the ForkJoinPool without the ability for some one to specify another ExecutorService.

When ever code uses Threading then the ability to control the underlying ExecutorService is super important.

Managing threadlocals and security managers and so on can be important some some people (it very much is at Atlassian where I work) and hence you need a way to inject your own ExecutorService into the mix.

For example notice how graphql.execution.ExecutorServiceExecutionStrategy takes an executor service say. This is done for that reason.

Make this code require an ExecutorService, default it to ForkJoinPool.defaultPool() and have the static methods also take an ExecutorService.

return userPromise;
}
};
You can use ``graphql.schema.AsynchronousDataFetcher.async(DataFetcher<T>)`` to wrap a ``DataFetcher`` so that it runs asynchronously.

This comment has been minimized.

@bbakerman

bbakerman Oct 25, 2017

Member

Please reinstate the original documentation. We want to show it more from first principles and how one might write their own async data fetcher

Feel free to add a futher section that might say something like

There is a helpful shortcur in graphql-java to create asynchronous data fetchers. 
Use ``graphql.schema.AsynchronousDataFetcher.async(DataFetcher<T>)`` to wrap a 
``DataFetcher``  as such .....
@RusticFlare

This comment has been minimized.

Contributor

RusticFlare commented Oct 25, 2017

@bbakerman Thanks for the feedback 👍 I'll get on it

@RusticFlare

This comment has been minimized.

Contributor

RusticFlare commented Oct 26, 2017

@bbakerman I've added the ability to set the Executor you use with the method AsynchronousDataFetcher.executeIn(Executor).

Used like this:

GraphQLFieldDefinition.newFieldDefinition()
        .dataFetcher(async(fooDataFetcher).executeIn(somePool))
        // ... rest of field definition
}
private final DataFetcher<T> wrappedDataFetcher;
private Executor executor = ForkJoinPool.commonPool();

This comment has been minimized.

@bbakerman

bbakerman Oct 26, 2017

Member

Sorry to be a pain but please make this final.

Have a constructor that takes a wrappedDF and another that takes a wrappedDF and a ExecutorService.

Use defaulting from 1 --> 2

Then have statics for both say

public static <T> AsynchronousDataFetcher<T> async(DataFetcher<T> wrappedDataFetcher) {
    return new AsynchronousDataFetcher<>(wrappedDataFetcher);
}

public static <T> AsynchronousDataFetcher<T> asyncWithExecutor(DataFetcher<T> wrappedDataFetcher, Executor executor) {
    return new AsynchronousDataFetcher<>(wrappedDataFetcher, executor);
}

This way the class is final and immutable but we have two ways of making it

Or make it return a new AsynchronousDataFetcher via the inner method

  AsynchronousDataFetcher adf = async(env  -> ...).usingExecutor(myExecutor)

where usingExecutor gives out a new AsynchronousDataFetcher copy

This comment has been minimized.

@RusticFlare

RusticFlare Oct 26, 2017

Contributor

Sure, no worries 👍

@RusticFlare

This comment has been minimized.

Contributor

RusticFlare commented Oct 26, 2017

@bbakerman I've gone with your first suggestion and added another static: asyncWithExecutor 🙂

@bbakerman

Thanks for this

@bbakerman bbakerman merged commit c59c31a into graphql-java:master Oct 27, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@RusticFlare RusticFlare deleted the RusticFlare:add-async-data-fetcher branch Oct 27, 2017

@RusticFlare

This comment has been minimized.

Contributor

RusticFlare commented Oct 27, 2017

@bbakerman Thanks for being patient with me 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment