Asynchronous execution strategy by dminkovsky · Pull Request #283 · graphql-java/graphql-java

dminkovsky · 2016-12-20T15:36:30Z

Following up with #242 and #202, this is a working asynchronous execution strategy:

GraphQLAsync graphql = new GraphQLAsync(
  schema,
  AsyncExecutionStrategy.serial(),
  AsyncExecutionStrategy.parallel()
);

String query = "{ name }";
CompletionStage<ExecutionResult> result = graphql.executeAsync(query);
result.thenAccept(executionResult -> {
  // use result //
})

Calls are made through GraphQLAsync, which extends GraphQL and provides an executeAsync method that returns CompletionStage<ExecutionResult>.

dminkovsky · 2016-12-26T02:04:03Z

src/main/java/graphql/execution/async/AsyncExecutionStrategy.java

This for loop needs to be replaced with a construct that properly closes over the vars passed to resolveFieldAsync

could make use of java8's stream.map.collect thing

Good call. Thank you. What do you think otherwise?

@hepin1989 replaced this with .collect(toMap()). Thanks again.

oexza · 2016-12-30T21:42:25Z

Good work! I think the last missing piece is a way to pass in a custom Executor in which the CompletableFuture's will execute.

Edit: On a second look, i see that DataFetchers would be allowed to return a CompletionStage, therefore an Executor can be used by the user in their DataFetcher implementations. So i think this PR is good to go.

crazyfrozenpenguin · 2016-12-31T05:51:58Z

@dminkovsky, I have just browsed through the code but will soon test it.

One feature that it would be great to have would be the concept of DataFetcher dependencies when executing in parallel.

A DataFetcher can depend on other DataFetchers or, a DataFetcher could unlock other DataFetchers.

dminkovsky · 2016-12-31T15:48:24Z

Awesome, thank you. There seems to be an issue where an error is swallowed. I am going to look into it today or tomorrow and write a test that confirms it. Also I need to make sure this strategy conforms to the issue described in #268 But I think all the non-error paths work fine. Re: the DataFetcher behavior you describe, if I understand you correctly I think that's how it's set up right now. If you can think of how to write a test that ensures fetchers execute in the correct order (that's important either way for accepting this PR) then I bet we could also describe what you're saying here in those terms. сб, 31 дек. 2016 г. в 0:51, crazyfrozenpenguin <[email protected]>:

…

@dminkovsky <https://github.com/dminkovsky>, I have just browsed through the code but will soon test it. One feature that it would be great to have would be the concept of DataFetcher dependencies when executing in parallel. A DataFetcher can depend on other DataFetchers or, a DataFetcher could unlock other DataFetchers. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#283 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AANWZdf_0XnBgcS6DKtVRVk3i87Yul__ks5rNe1_gaJpZM4LR92_> .

dminkovsky · 2017-01-01T00:38:20Z

@crazyfrozenpenguin identified and fixed the above-mentioned bug. Also fixed another issue. The update is forced pushed to this PR. Happy New Year.

dminkovsky · 2017-01-01T01:12:44Z

By the way, I also updated the SNAPSHOT version. It was 2.3.0-SNAPSHOT, but 2.3.0 is now released so I just set it to 3.0.0-SNAPSHOT which should work until this is merged or otherwise closed.

oexza · 2017-01-05T02:46:31Z

How do you intend to release this? will the project move to java 8? or will you release this as a seperate project?

bmsantos · 2017-01-05T03:46:50Z

What's the reason for Java 6? The only release with public updates is Java 8:
http://www.oracle.com/technetwork/java/eol-135779.html

andimarek · 2017-01-05T07:11:51Z

@bmsantos The reason we still support Java 6 is that a lot of companies are still using it, even if there are no updates for it.

dminkovsky · 2017-01-05T13:43:55Z

@oexza yeah the plan is for a separate artifact. I have this PR here now to aid in peer review.

As @andimarek says, there are still people on Java 6 using this, which I think is really cool. It would be nice to somehow isolate a 1.6 and 1.7 compatible core that they can keep using.

What do you think? cc/ @bmsantos

bmsantos · 2017-01-05T14:18:58Z

@dminkovsky and @andimarek, if you really want to keep offering support for 1.6, then yes, I don't think that there's any other option other than extract async code into its own jar - maybe graphql-java-async?

dminkovsky · 2017-01-05T14:20:29Z

@bmsantos Yeah that's the name I was thinking.

bmsantos · 2017-01-05T14:28:17Z

Breaking this on its own jar might actually be a much better approach since async related releases/updates will be easier to do.

Let me know if you need any help on this.

dminkovsky · 2017-01-05T14:44:07Z

That would be greatly appreciated!

Breaking this on its own jar might actually be a much better approach since async related releases/updates will be easier to do.

Yeah, that's what I am thinking too. For example, fixing #268 is a bit difficult because the maintainers need to verify that the problem does not exist in all of the execution strategies (simple, executor service, and batched). #284 is related: with a clearer demarcation of API vs SPI, I think this would be much easier.

Cafeinoman · 2017-01-19T19:04:41Z

Is there a plan a plan / date to release this? It would be really helpful on an actual project...

dminkovsky · 2017-01-20T15:54:27Z

Hi @Cafeinoman. Thanks for posting.

I've had to take a few weeks away from this while I launch a project. I am using this in this project, so I will need to get back to it soon.

I think this PR is in okay shape except for correct handling of null non-nullable field (#268). That issue needs to be fixed in the other execution strategies as well. I don't think it makes sense to "release" this until those issues are addressed.

In the mean time, have you played with this branch? If you use it, please report any issues or PR improvements.

dminkovsky · 2017-01-20T16:00:51Z

Regarding #268, if anyone wants to tackle it:

Please review 6.4.4 Errors and Non-Nullability in the spec.
Please review graphql-js execution and see how null non-nullable fields are handled correctly.

As you will see, graphql-js execution tracks ResponsePath during execution, which aids in producing good errors. To me it appeared that to make our implementation work to spec and like graphql-js, we should not create and add errors during field resolution, but later during completion. We should also keep a response path, so that the errors we create can have that path in the error!

bmsantos · 2017-01-20T17:05:27Z

I'm trying to get some free cycles to put out a small version of a project that exercises graphql with Vert.x. This will help me (and maybe others) to quickly exercise and test the async move to a jar. I'll try to put a first sample by the end of this weekend.

graphql-java async implmentation should allow for non-default CompletableFeatures as some users/frameworks/toolkits want to use their own thread pools.

dminkovsky · 2017-02-22T15:36:33Z

Cool. All set.

…

On Wed, Feb 22, 2017 at 10:23 AM, Dmitry Minkovsky ***@***.***> wrote: Starting now, hopefully won't take too long. вт, 21 февр. 2017 г. в 14:46, Bruno Santos ***@***.***>: > Sounds good. If you want to rebase, that will be great. Otherwise I can > always rebase it myself. > > Thanks > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#283 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AANWZVmge-Tg_8srM-3ABJhw46pEFwTXks5rez73gaJpZM4LR92_> > . >

bmsantos · 2017-02-23T15:34:50Z

Thanks, will get to it ASAP.

bmsantos · 2017-02-24T03:47:30Z

@dminkovsky, I've created the graphql-java-async project here. In addition, a PR has been placed against your fork of graphql-java async branch.

Let me know if you find any issues and/or have any questions.

dminkovsky · 2017-02-24T14:34:11Z

Thank you @bmsantos! I will take a look soon.

…

On Thu, Feb 23, 2017 at 10:47 PM, Bruno Santos ***@***.***> wrote: @dminkovsky <https://github.com/dminkovsky>, I've created the graphql-java-async project here <https://github.com/bmsantos/graphql-java-async>. In addition, a PR <dminkovsky#3> has been placed against your fork of graphql-java async branch. Let me know if you find any issues and/or have any questions. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#283 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AANWZQndnTwSgomrQfTeOpBZffeFX8Irks5rflLSgaJpZM4LR92_> .

denvned · 2017-03-11T18:38:37Z

src/main/java/graphql/GraphQLAsync.java

+        super(graphQLSchema, queryStrategy, mutationStrategy);
+    }
+
+    public CompletionStage<ExecutionResult> executeAsync(String requestString) {


It would be more convenient if it returned CompletableFuture here.

dminkovsky · 2017-03-16T01:23:16Z

You can do #toCompletableFuture() on a CompletionStage. Sorry for the late reply.

…

On Sat, Mar 11, 2017 at 1:38 PM, Denis Nedelyaev ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/main/java/graphql/GraphQLAsync.java <#283 (comment)> : > + + private static Logger log = LoggerFactory.getLogger(GraphQLAsync.class); + + public GraphQLAsync(GraphQLSchema graphQLSchema) { + super(graphQLSchema); + } + + public GraphQLAsync(GraphQLSchema graphQLSchema, ExecutionStrategy queryStrategy) { + super(graphQLSchema, queryStrategy); + } + + public GraphQLAsync(GraphQLSchema graphQLSchema, ExecutionStrategy queryStrategy, ExecutionStrategy mutationStrategy) { + super(graphQLSchema, queryStrategy, mutationStrategy); + } + + public CompletionStage<ExecutionResult> executeAsync(String requestString) { It would be more convenient if it returned CompletableFuture here. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#283 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AANWZcQhqnd3ZDc5q0FTTazUB7c0Cb0Vks5rkuotgaJpZM4LR92_> .

andimarek · 2017-03-16T01:25:56Z

FYI: We are discussing an upgrade to Java 8: #338

dminkovsky · 2017-03-16T14:40:39Z

@andimarek yeah I've kind of been following that. Just still trying to finish my project which is woefully overdue :(. Looking forward to crawling out of my hole and rejoining the world.

andimarek · 2017-05-06T14:33:44Z

fyi: we will look at that topic for version 4

andimarek · 2017-06-19T01:53:23Z

Please have a look at #324 for the overall disucssion

andimarek · 2017-07-05T05:47:33Z

closing it, it will not get merged: we solved the different aspects of this PR in the meantime and we we just merged #380 as a first step in supporting async execution.

Thanks a lot @dminkovsky

dminkovsky commented Dec 26, 2016

View reviewed changes

dminkovsky force-pushed the async branch 4 times, most recently from da0b587 to 0b2d9d3 Compare January 1, 2017 00:36

dminkovsky force-pushed the async branch 3 times, most recently from 70174b3 to 9ed6aee Compare January 1, 2017 00:53

dminkovsky force-pushed the async branch 2 times, most recently from 85c8939 to 4257f66 Compare January 4, 2017 23:55

dminkovsky mentioned this pull request Jan 4, 2017

Should allow for specific CompletableFuture implementations dminkovsky/graphql-java#1

Merged

dminkovsky mentioned this pull request Jan 20, 2017

execution path propagation for better execution errors #304

Closed

dminkovsky force-pushed the async branch from 4257f66 to ebb2a8c Compare January 30, 2017 19:48

dminkovsky and others added 13 commits February 22, 2017 10:34

Async execution strategy

3af03bb

Should allow for specific CompletableFuture implementations

dd574c3

graphql-java async implmentation should allow for non-default CompletableFeatures as some users/frameworks/toolkits want to use their own thread pools.

Test for null with isNull

cfc56c7

Static import Arrays.asList

101a9d6

Removed unnecessary @SuppressWarnings

3c4f59f

executeInParallel correctly handles null results

182845d

executeInParallel handles exceptions

f3c5366

Removed unnecessary map copy

54d8c02

Capture propagating NonNullException

a585ef7

Cosmetic changes to async strategy spec

d551919

Fixup after rebase: DataFetchingEnvironment now an interface

5d88bc5

Fixup after rebase: ExecutionContext now has an ExecutionId

4b063a1

Should set execution ID

3e4eee5

dminkovsky force-pushed the async branch from 470ecda to 3e4eee5 Compare February 22, 2017 15:35

bmsantos mentioned this pull request Feb 28, 2017

Move async to graphql-java-async project dminkovsky/graphql-java#3

Open

bmsantos mentioned this pull request Mar 9, 2017

Make Java8 the base library for graphql-java #338

Closed

denvned reviewed Mar 11, 2017

View reviewed changes

andimarek added this to the 4.0.0 milestone May 6, 2017

andimarek mentioned this pull request May 22, 2017

Asynchronous DataFetcher? #37

Closed

andimarek closed this Jul 5, 2017

Conversation

dminkovsky commented Dec 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dminkovsky Dec 26, 2016

Choose a reason for hiding this comment

Uh oh!

He-Pin Dec 26, 2016

Choose a reason for hiding this comment

Uh oh!

dminkovsky Dec 28, 2016

Choose a reason for hiding this comment

Uh oh!

dminkovsky Jan 1, 2017

Choose a reason for hiding this comment

Uh oh!

oexza commented Dec 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crazyfrozenpenguin commented Dec 31, 2016

Uh oh!

dminkovsky commented Dec 31, 2016 via email

Uh oh!

dminkovsky commented Jan 1, 2017

Uh oh!

dminkovsky commented Jan 1, 2017

Uh oh!

oexza commented Jan 5, 2017

Uh oh!

bmsantos commented Jan 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andimarek commented Jan 5, 2017

Uh oh!

dminkovsky commented Jan 5, 2017

Uh oh!

bmsantos commented Jan 5, 2017

Uh oh!

dminkovsky commented Jan 5, 2017

Uh oh!

bmsantos commented Jan 5, 2017

Uh oh!

dminkovsky commented Jan 5, 2017

Uh oh!

Cafeinoman commented Jan 19, 2017

Uh oh!

dminkovsky commented Jan 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dminkovsky commented Jan 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmsantos commented Jan 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dminkovsky commented Feb 22, 2017 via email

Uh oh!

bmsantos commented Feb 23, 2017

Uh oh!

bmsantos commented Feb 24, 2017

Uh oh!

dminkovsky commented Feb 24, 2017 via email

Uh oh!

denvned Mar 11, 2017

Choose a reason for hiding this comment

Uh oh!

dminkovsky commented Mar 16, 2017 via email

Uh oh!

andimarek commented Mar 16, 2017

Uh oh!

dminkovsky commented Mar 16, 2017

Uh oh!

andimarek commented May 6, 2017

Uh oh!

andimarek commented Jun 19, 2017

Uh oh!

andimarek commented Jul 5, 2017

Uh oh!

Reviewers

Assignees

dminkovsky commented Dec 20, 2016 •

edited

Loading

oexza commented Dec 30, 2016 •

edited

Loading

bmsantos commented Jan 5, 2017 •

edited

Loading

dminkovsky commented Jan 20, 2017 •

edited

Loading

dminkovsky commented Jan 20, 2017 •

edited

Loading

bmsantos commented Jan 20, 2017 •

edited

Loading