AsynchronousExecutionStrategy without changing the interface of GraphQL#481
AsynchronousExecutionStrategy without changing the interface of GraphQL#481charliechang wants to merge 2 commits intographql-java:masterfrom
Conversation
|
I havent looked that code implementation yet but I think we SHOULD break the API. It will put is inline with the reference implementation which gives back a Promise aka a CompleteableFuture I have a PR for that to show it - but its not a true asynch ACTUAL implementation but rather just breaking API shape |
|
This is the PR in question I think strongly that we should have a Promise based return value before we add an actual asynch execution strategy |
bbakerman
left a comment
There was a problem hiding this comment.
I think this is a great PR.
In fact it begs the question that if we adopted this, that the base SimpleExecutionStrategy becomes this with .join() as the behavior say.
I do think that we should get the other Promise PR in place before adding this however
| ExecutionParameters parameters) throws NonNullableFieldWasNullException { | ||
|
|
||
| Map<String, List<Field>> fields = parameters.fields(); | ||
| Map<String,Object> results = Collections.synchronizedMap(new HashMap<>()); |
There was a problem hiding this comment.
ConcurrentHashMap is a better choice than synchronizedMap
|
|
||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Mostly pondering here.
I wonder if we should make the complete value list truly async. Should it join back as one result at this point?
Is there any disadvantage is leaving it purely async for many items in a list? Is there any expectation that is this point you have resolved the value so it can be sent onto future calls?
| import graphql.execution.instrumentation.InstrumentationContext; | ||
| import graphql.execution.instrumentation.parameters.FieldFetchParameters; | ||
| import graphql.execution.instrumentation.parameters.FieldParameters; | ||
| import graphql.language.Field; |
There was a problem hiding this comment.
we dont do * imports. Full imports please
| handleDataFetchingException(executionContext, fieldDef, argumentValues, new ExecutionException(th)); | ||
| fetchCtx.onEnd(th); | ||
| } | ||
|
|
There was a problem hiding this comment.
The Instrumentation is not called back here and hence is lost
fetchCtx.onEnd(resolvedValue);
- Add in intrumentation call
| import static graphql.execution.FieldCollectorParameters.newParameters; | ||
| import static graphql.execution.TypeInfo.newTypeInfo; | ||
|
|
||
| public class AsynchronousExecutionStrategy extends ExecutionStrategy { |
There was a problem hiding this comment.
This comment is an aside to this PR btw.
There is a lot of repeated code in here from the base ExecutionStrategy. Perhaps we should make a ExecutionStrategyHelper and use composition instead of inheritance to allow better reuse??
Hard problem I know
| Object result = parameters.source(); | ||
| GraphQLType fieldType = parameters.typeInfo().type(); | ||
|
|
||
| if (result == null) { |
There was a problem hiding this comment.
fyi - there are a few PRS in the pipe that will make this code break. But thats often always the case
| then: | ||
| assert result == expected | ||
| } | ||
| } |
There was a problem hiding this comment.
Are we trying to say that the first things to be resolved (grand) is likely to be the last thing to be completed (50ms) and hence it come back in the right order.?
There was a problem hiding this comment.
Just trying to work out what the tests really proves
|
@charliechang Please have a look at #380 for a first start. |
|
closing it, because we decided to make @charliechang thanks a lot for this PR and the work you have put into it! |
Here is the PR which implemented the
AsynchronousExecutionStrategyby Java 8CompletableFuturewithout changing the interface in GraphQL. The returns ofGraphQL.getData()will be an instance ofCompletionStage<Map>Please let me know your thought