Feature/183 refactor abstract http servlet #217
Conversation
|
I can't deep dive into it until Monday but after a quick read I like the direction |
| // List<ExecutionInput> executionIds = batchedInvocationInput.stream() | ||
| // .map(GraphQLSingleInvocationInput::getExecutionInput) | ||
| // .collect(Collectors.toList()); | ||
| // Supplier<Instrumentation> configuredInstrumentation = contextSetting |
There was a problem hiding this comment.
@jmccaull I've created a separate GraphQLBuilder to create the GraphQL object to run the queries against. I thought it would improve performance if you'd just create that once instead of on every query. This is the only part though that I couldn't easily refactor. Any thoughts on this?
I'm also working on a webflux starter, which means we'd have to support a CompletableFuture result for batches too.
There was a problem hiding this comment.
It has been a while since we did any profiling, I don't remember this showing up at all as a hotspot or anything. The documentation on the GraphQl object states "Building this object is very cheap and can be done on each execution if necessary...". That being said reusing it would be nice, cheap is relative and documentation gets outdated. It would certainly remove a lot of complexity from invoking a query too.
What I see making this hard to refactor is the instrumentation - I wrote it around the object being recreated for each execution while trying to reuse as much of the original dispatch code as possible, that seems like a mistake from this perspective. The behavior for each context settings is so different would it make sense to create a separate invoker for each? It seems like the request scoped logic would need a handle not only to the GraphQl object but to the instrumentation as well to set it up for each request. At that point too it is probably worth revisiting the tracking approach abstraction, maybe making two distinct instrumentations.
Let me know if you want any help with the lift, etc. It seems like a fairly large one
There was a problem hiding this comment.
I decided to create it on the fly anyway, but still refactoring it a bit.
…tp-servlet' into feature/183-refactor-abstract-http-servlet
@jmccaull @drupalspring
First step in refactoring the servlet. Any comments/contributions are appreciated.