Repository navigation
GQLGW-5297-optimise-incremental-part-execution-for-defer-requests #4174
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
Changes from 3 commits
dd8313b
7e8d69a
a5c458e
50169f0
0f365b5
18207de
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| package graphql.execution.incremental; | ||
|
|
||
|
|
||
| import graphql.Internal; | ||
| import org.jspecify.annotations.NullMarked; | ||
|
|
||
| /** | ||
| * GraphQLContext keys for controlling incremental execution behavior. | ||
| */ | ||
| @Internal | ||
| @NullMarked | ||
| public final class IncrementalExecutionContextKeys { | ||
| private IncrementalExecutionContextKeys() { | ||
| } | ||
|
|
||
| /** | ||
| * Enables eager start of @defer processing so defered work runs before the initial result is computed. | ||
| * Defaults to false. | ||
| * <p> | ||
| * Expects a boolean value. | ||
| */ | ||
| public static final String ENABLE_EAGER_DEFER_START = "__GJ_enable_eager_defer_start"; | ||
|
|
||
| } | ||
|
|
||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to create a new entry in the unusual config - lets put it in
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ import graphql.ExecutionResult | |
| import graphql.ExperimentalApi | ||
| import graphql.GraphQL | ||
| import graphql.GraphqlErrorBuilder | ||
| import graphql.GraphQLContext | ||
| import graphql.TestUtil | ||
| import graphql.execution.DataFetcherResult | ||
| import graphql.execution.pubsub.CapturingSubscriber | ||
|
|
@@ -27,6 +28,8 @@ import spock.lang.Specification | |
| import spock.lang.Unroll | ||
|
|
||
| import java.util.concurrent.CompletableFuture | ||
| import java.util.concurrent.CountDownLatch | ||
| import java.util.concurrent.TimeUnit | ||
| import java.util.concurrent.atomic.AtomicInteger | ||
|
|
||
| import static graphql.schema.idl.TypeRuntimeWiring.newTypeWiring | ||
|
|
@@ -1726,6 +1729,142 @@ class DeferExecutionSupportIntegrationTest extends Specification { | |
|
|
||
| } | ||
|
|
||
| def "eager defer starts before initial result completes when ENABLE_EAGER_DEFER_START"() { | ||
| given: | ||
| def deferStarted = new CountDownLatch(1) | ||
| def allowDeferredComplete = new CountDownLatch(1) | ||
|
|
||
| def runtimeWiring = RuntimeWiring.newRuntimeWiring() | ||
| .type(newTypeWiring("Query") | ||
| .dataFetcher("post", resolve([id: "1001"])) | ||
| ) | ||
| .type(newTypeWiring("Query").dataFetcher("hello", resolve("world", 4000))) | ||
| .type(newTypeWiring("Post").dataFetcher("summary", { env -> | ||
| deferStarted.countDown() | ||
| allowDeferredComplete.await(2, TimeUnit.SECONDS) | ||
| CompletableFuture.completedFuture("A summary") | ||
| } as DataFetcher)) | ||
| .type(newTypeWiring("Item").typeResolver(itemTypeResolver())) | ||
| .build() | ||
|
|
||
| def schema = TestUtil.schema(schemaSpec, runtimeWiring) | ||
| .transform({ b -> b.additionalDirective(Directives.DeferDirective) }) | ||
| def testGraphQL = GraphQL.newGraphQL(schema).build() | ||
|
|
||
| def ctx = GraphQLContext.newContext().build() | ||
| ctx.put(ExperimentalApi.ENABLE_INCREMENTAL_SUPPORT, true) | ||
| ctx.put(IncrementalExecutionContextKeys.ENABLE_EAGER_DEFER_START, true) | ||
|
|
||
| def query = ''' | ||
| query { | ||
| hello | ||
| ... @defer { post { summary } } | ||
| } | ||
| ''' | ||
|
|
||
| when: | ||
| def executionInput = ExecutionInput.newExecutionInput() | ||
| .graphQLContext([(ExperimentalApi.ENABLE_INCREMENTAL_SUPPORT): true, (IncrementalExecutionContextKeys.ENABLE_EAGER_DEFER_START): true]) | ||
| .query(query) | ||
| .build() | ||
| def execFuture = CompletableFuture.supplyAsync { | ||
| testGraphQL.execute(executionInput) | ||
| } | ||
|
|
||
| then: | ||
| // Deferred fetcher starts while initial result is still computing | ||
| assert deferStarted.await(2000, TimeUnit.MILLISECONDS) | ||
| assert !execFuture.isDone() | ||
|
|
||
| when: | ||
| allowDeferredComplete.countDown() | ||
| def initialResult = execFuture.join() as IncrementalExecutionResult | ||
|
|
||
| then: | ||
| assert initialResult.toSpecification() == [ | ||
| data : [hello: "world"], | ||
| hasNext: true | ||
| ] | ||
|
|
||
| when: | ||
| def sub = new CapturingSubscriber<DelayedIncrementalPartialResult>() | ||
| initialResult.incrementalItemPublisher.subscribe(sub) | ||
|
|
||
| then: | ||
| Awaitility.await().untilTrue(sub.isDone()) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needs an assert on the contents of the Publisher |
||
| } | ||
|
|
||
|
|
||
| def "incremental starts only after initial result when eager start disabled"() { | ||
| given: | ||
| def deferStarted = new CountDownLatch(1) | ||
| def allowDeferredComplete = new CountDownLatch(1) | ||
|
|
||
| def runtimeWiring = RuntimeWiring.newRuntimeWiring() | ||
| .type(newTypeWiring("Query") | ||
| .dataFetcher("post", resolve([id: "1001"])) | ||
| ) | ||
| .type(newTypeWiring("Query").dataFetcher("hello", resolve("world", 300))) | ||
| .type(newTypeWiring("Post").dataFetcher("summary", { env -> | ||
| deferStarted.countDown() | ||
| allowDeferredComplete.await(2, TimeUnit.SECONDS) | ||
| CompletableFuture.completedFuture("A summary") | ||
| } as DataFetcher)) | ||
| .type(newTypeWiring("Item").typeResolver(itemTypeResolver())) | ||
| .build() | ||
|
|
||
| def schema = TestUtil.schema(schemaSpec, runtimeWiring) | ||
| .transform({ b -> b.additionalDirective(Directives.DeferDirective) }) | ||
| def testGraphQL = GraphQL.newGraphQL(schema).build() | ||
|
|
||
| def query = ''' | ||
| query { | ||
| hello | ||
| ... @defer { post { summary } } | ||
| } | ||
| ''' | ||
|
|
||
| when: | ||
| def executionInput = ExecutionInput.newExecutionInput() | ||
| .graphQLContext([(ExperimentalApi.ENABLE_INCREMENTAL_SUPPORT): true]) // no eager flag | ||
| .query(query) | ||
| .build() | ||
| def execFuture = CompletableFuture.supplyAsync { | ||
| testGraphQL.execute(executionInput) | ||
| } | ||
|
|
||
| then: | ||
| assert !deferStarted.await(100, TimeUnit.MILLISECONDS) | ||
| assert !execFuture.isDone() | ||
|
|
||
| when: | ||
| def initialResult = execFuture.join() as IncrementalExecutionResult | ||
|
|
||
| then: | ||
| assert initialResult.toSpecification() == [ | ||
| data : [hello: "world"], | ||
| hasNext: true | ||
| ] | ||
| assert deferStarted.count == 1 // still not started, no subscriber yet | ||
|
|
||
| when: | ||
| allowDeferredComplete.countDown() | ||
| def incrementalResults = getIncrementalResults(initialResult) | ||
|
|
||
| then: | ||
| incrementalResults == [ | ||
| [ | ||
| hasNext : false, | ||
| incremental: [ | ||
| [ | ||
| path: [], | ||
| data: [post: [summary: "A summary"]] | ||
| ] | ||
| ] | ||
| ] | ||
| ] | ||
| } | ||
|
llin2 marked this conversation as resolved.
Resolved
|
||
|
|
||
|
|
||
| private ExecutionResult executeQuery(String query) { | ||
| return this.executeQuery(query, true, [:]) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This call here is only need as a side effect to create the memoizing supplier (of the Publisher)
I think we should invent a new method that does that and hides the side effect
executionContext.getIncrementalCallState().startEarlyDraining();There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hang on you have that - startDrainingNow() and it calls publisher.get()
I dont think executionContext.getIncrementalCallState().startDeferredCalls(); is needed