Skip to content
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

Enables NIO-based Concurrent Execution #164

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

NeedleInAJayStack
Copy link
Member

The existing concurrent executor was a mix of DispatchQueue.async calls and NIO futures, and was only used on an opt-in basis.

This makes the following changes:

  1. Adds a fully NIO-based concurrent executor
  2. Makes it the default executor used on query and subscription operations
  3. Deprecates APIs that allow specifying particular executors, as well as the DispatchQueue-based executor.

This should better parallelize GraphQL queries by default, resulting in improved performance.

Changes to default values are typically considered major, however this seems to be an exception. The GraphQL spec states the resolvers of queries must be side-effect free and idempotent, the execution order must not affect the result, which means that this can be considered a minor based on the resolver idempotency agreement.

The reasoning for the deprecation is that the JavaScript reference implementation does not expose executor implementations to the caller, instead always executing query and subscription concurrently and mutation serially. Furthermore, Graphiti does not allow customization of executors, implying that it is a rarely used feature.

@NeedleInAJayStack NeedleInAJayStack self-assigned this Jan 26, 2025
@NeedleInAJayStack NeedleInAJayStack changed the title Enables nio-based Concurrent Execution Enables NIO-based Concurrent Execution Jan 26, 2025
@NeedleInAJayStack NeedleInAJayStack force-pushed the feat/concurrent-execution branch from b481087 to 24b5606 Compare January 26, 2025 05:51
@paulofaria
Copy link
Member

Is this good for review?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants