DO NOT MERGE : This POC shows what a new GraphQLEngine might look like #3348
Conversation
bbakerman
added
breaking change
| * The parameters that are passed to {@link GraphQLEngine}s | ||
| */ | ||
| @PublicApi | ||
| public class EngineParameters { |
There was a problem hiding this comment.
This is pretty much a copy of ExecutionStrategyParameters but I felt that is too tied to the old ES way
|
|
||
| public interface GraphQLEngine { | ||
|
|
||
| Instrumentation getInstrumentation(); |
There was a problem hiding this comment.
Instrumentation is engine specific - it has to be
| if (logNotSafe.isDebugEnabled()) { | ||
| logNotSafe.debug("Executing '{}' query operation: '{}' using '{}' execution strategy", executionContext.getExecutionId(), operation, executionStrategy.getClass().getName()); | ||
| } | ||
| result = executionStrategy.execute(executionContext, parameters); |
There was a problem hiding this comment.
this now needs to happen inside the engine. They pick what to execute
| @@ -43,24 +44,24 @@ | |||
| */ | |||
| @SuppressWarnings("deprecation") | |||
| @PublicApi | |||
| public class ChainedInstrumentation implements Instrumentation { | |||
| public class ChainedInstrumentation implements OriginalEngineInstrumentation { | |||
There was a problem hiding this comment.
this area is where I need the most feedback - nominally a ChainedInstrumentation that is NOT on the original would be useful but its always going to have to have a specific class.
Maybe I make a ``ChainedOriginalInstrumentation extends ChainedInstrumentation` some how
| @@ -199,183 +199,6 @@ default InstrumentationContext<ExecutionResult> beginExecuteOperation(Instrument | |||
| return beginExecuteOperation(parameters.withNewState(state)); | |||
| } | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
The removed methods are specific to the ES code base
…tion is done differently now - they can be passed around but detection is done when used
…tion is done differently now - tweaked tests
…tion is done differently now - @internal on ES
|
Lets test this design (or any ES design change) with:
Other notes:
|
This PR introduces GraphQlEngine interface that represents execution better than the
ExecutionStrategycode.It's truly generic and leaves the implementation up to the implementor - unlike the abstract
ExecutionStrategycode that requires you to use its exact algo.Of course Instrumentation of the field fetches etc... are really tied to how the engine runs and hence this breaks the Instrumentation of the parts before "engine execution" out and creates an engine specific
OriginalEngineInstrumentationfor the ES specific parts.The naming here sucks in many ways as do the packages but I wanted to make the smallest useful breaking change. We can discuss if we want to break things by moving things around more. At the end of the day the instrumentation of engine is specific to the engine. It just is.
This is discussed in #3331
This is currently on failing these tests
they are mostly because they deeply test the innards of code that has changed and I didnt spend the time yet to re-test with the new behavior.
So in a general sense most of the graphql code base is working.