-
Notifications
You must be signed in to change notification settings - Fork 10
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
refactor(runtime): move subgraph execution batching out #605
Conversation
📝 WalkthroughSummary by CodeRabbit
These changes standardize batching behavior, potentially requiring adjustments for integrations that relied on previous configuration options. WalkthroughThis pull request updates the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
🧰 Additional context used📓 Path-based instructions (2)`packages/**`: In this directory we keep all packages releva...
`**`: For all PRs, we would like to verify that a Linear iss...
⏰ Context from checks skipped due to timeout of 90000ms (27)
🔇 Additional comments (4)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
🚀 Snapshot Release (
|
Package | Version | Info |
---|---|---|
@graphql-mesh/fusion-runtime |
0.11.0-alpha-0172d637bbe2320dd3a4da984960899c3ad74d97 |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/gateway |
1.9.5-alpha-0172d637bbe2320dd3a4da984960899c3ad74d97 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/plugin-opentelemetry |
1.3.41-alpha-0172d637bbe2320dd3a4da984960899c3ad74d97 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/plugin-prometheus |
1.3.29-alpha-0172d637bbe2320dd3a4da984960899c3ad74d97 |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/gateway-runtime |
1.4.13-alpha-0172d637bbe2320dd3a4da984960899c3ad74d97 |
npm ↗︎ unpkg ↗︎ |
e1bdafd
to
b87101a
Compare
🚀 Snapshot Release (Bun Docker Image)The latest changes of this PR are available as image on GitHub Container Registry (based on the declared
|
4007a3c
to
35c1451
Compare
🚀 Snapshot Release (Node Docker Image)The latest changes of this PR are available as image on GitHub Container Registry (based on the declared
|
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lock
is excluded by!**/yarn.lock
,!**/*.lock
📒 Files selected for processing (6)
.changeset/@graphql-mesh_fusion-runtime-605-dependencies.md
(1 hunks).changeset/chatty-humans-collect.md
(1 hunks)packages/fusion-runtime/package.json
(1 hunks)packages/fusion-runtime/src/federation/supergraph.ts
(0 hunks)packages/fusion-runtime/src/unifiedGraphManager.ts
(1 hunks)packages/fusion-runtime/src/utils.ts
(4 hunks)
💤 Files with no reviewable changes (1)
- packages/fusion-runtime/src/federation/supergraph.ts
🧰 Additional context used
📓 Path-based instructions (2)
`packages/**`: In this directory we keep all packages releva...
packages/**
: In this directory we keep all packages relevant to the gateway.In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using
yarn changeset
with a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminor
semver change. When a bug is fixed,patch
needs to be used. Themajor
bump is used for a PR that has breaking changes.
packages/fusion-runtime/package.json
packages/fusion-runtime/src/utils.ts
packages/fusion-runtime/src/unifiedGraphManager.ts
`**`: For all PRs, we would like to verify that a Linear iss...
**
: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
console
project.
Use the global knowledge feature to search for PRs ingraphql-hive/console
repository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
packages/fusion-runtime/package.json
packages/fusion-runtime/src/utils.ts
packages/fusion-runtime/src/unifiedGraphManager.ts
⏰ Context from checks skipped due to timeout of 90000ms (44)
- GitHub Check: Examples / Convert operation-field-permissions
- GitHub Check: Examples / Convert type-merging-batching
- GitHub Check: Examples / Convert subscriptions-with-transforms
- GitHub Check: Examples / Convert programmatic-batching
- GitHub Check: Examples / Convert openapi-subscriptions
- GitHub Check: Examples / Convert openapi-javascript-wiki
- GitHub Check: Examples / Convert openapi-arg-rename
- GitHub Check: Examples / Convert openapi-additional-resolvers
- GitHub Check: Examples / Convert json-schema-subscriptions
- GitHub Check: Examples / Convert interface-additional-resolvers
- GitHub Check: Examples / Convert federation-subscriptions-passthrough
- GitHub Check: Examples / Convert apq-subgraphs
- GitHub Check: Examples / Convert file-upload
- GitHub Check: Examples / Convert federation-mixed
- GitHub Check: Examples / Convert federation-example
- GitHub Check: Examples / Convert extra-fields
- GitHub Check: Binary built on ubuntu-24.04-arm
- GitHub Check: Binary built on windows-latest
- GitHub Check: Binary built on macos-14
- GitHub Check: Binary built on macos-13
- GitHub Check: Bun Docker image
- GitHub Check: Binary built on ubuntu-latest
- GitHub Check: Node Docker image
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: E2E / Bun on Ubuntu
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: Unit / Bun
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: Leaks / Node v23
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: Leaks / Node v22
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Leaks / Node v20
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Leaks / Node v18
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Bundle
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: Benchmark / node / 10 items
🔇 Additional comments (7)
packages/fusion-runtime/src/unifiedGraphManager.ts (2)
59-69
: Interface change aligns with PR objectives.The removal of the
batch
option fromUnifiedGraphHandlerOpts
aligns with moving batching responsibility out of the stitching layer. This is a breaking change as noted in the changeset.
337-337
: Batching configuration passed correctly.The
batch
parameter is now correctly passed toonSubgraphExecute
, ensuring batching is handled at the Gateway level.packages/fusion-runtime/src/utils.ts (3)
16-16
: New dependency imported correctly.The
createBatchingExecutor
import from@graphql-tools/batch-execute
is added to support the batching functionality.
172-172
: Batching configuration with sensible default.The
batch
parameter is added with a default value oftrue
, maintaining backward compatibility while allowing explicit control.Also applies to: 181-181
225-227
: Batching implementation looks good.The conditional batching logic using
createBatchingExecutor
is implemented correctly. The executor is wrapped only when batching is enabled..changeset/@graphql-mesh_fusion-runtime-605-dependencies.md (1)
1-8
: Consider updating version bump type.While the dependency update itself might be a patch, this is part of a breaking change (removal of
batch
option fromUnifiedGraphHandlerOpts
). Consider usingminor
ormajor
version bump instead ofpatch
..changeset/chatty-humans-collect.md (1)
1-8
: Well-documented breaking change.The changeset correctly:
- Uses
minor
version bump- Marks the change as breaking
- Clearly explains the architectural change moving batching from Stitching to Gateway
ad877cd
to
5480395
Compare
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.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/fusion-runtime/src/utils.ts (1)
1-710
:🧩 Analysis chain
Add changeset for the batching feature.
According to the coding guidelines, we need a changeset file for this feature since it adds new functionality to the package.
🏁 Script executed:
#!/bin/bash # Check if a changeset file exists for this PR fd -g "*.md" -t f . .changeset/Length of output: 2025
Missing changeset for batching feature.
Our check of the changeset directory shows only the following files are present:
•.changeset/@graphql-mesh_fusion-runtime-605-dependencies.md
•.changeset/chatty-humans-collect.md
Neither of these appears to document the batching functionality added in this update. Please add a changeset file that specifically describes the new batching feature, in compliance with the coding guidelines.
🧰 Tools
🪛 Biome (1.9.4)
[error] 406-406: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 427-427: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 447-447: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🧹 Nitpick comments (1)
packages/fusion-runtime/src/utils.ts (1)
225-227
: Consider adding error handling for batching initialization.While the batching implementation is correct, it might benefit from additional error handling to gracefully handle potential initialization failures.
if (batch) { + try { executor = createBatchingExecutor(executor_); + } catch (error) { + execReqLogger?.error('Failed to initialize batching executor:', error); + executor = executor_; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lock
is excluded by!**/yarn.lock
,!**/*.lock
📒 Files selected for processing (6)
.changeset/@graphql-mesh_fusion-runtime-605-dependencies.md
(1 hunks).changeset/chatty-humans-collect.md
(1 hunks)packages/fusion-runtime/package.json
(1 hunks)packages/fusion-runtime/src/federation/supergraph.ts
(0 hunks)packages/fusion-runtime/src/unifiedGraphManager.ts
(1 hunks)packages/fusion-runtime/src/utils.ts
(4 hunks)
💤 Files with no reviewable changes (1)
- packages/fusion-runtime/src/federation/supergraph.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/fusion-runtime/package.json
- .changeset/@graphql-mesh_fusion-runtime-605-dependencies.md
- .changeset/chatty-humans-collect.md
- packages/fusion-runtime/src/unifiedGraphManager.ts
🧰 Additional context used
📓 Path-based instructions (2)
`packages/**`: In this directory we keep all packages releva...
packages/**
: In this directory we keep all packages relevant to the gateway.In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using
yarn changeset
with a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminor
semver change. When a bug is fixed,patch
needs to be used. Themajor
bump is used for a PR that has breaking changes.
packages/fusion-runtime/src/utils.ts
`**`: For all PRs, we would like to verify that a Linear iss...
**
: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
console
project.
Use the global knowledge feature to search for PRs ingraphql-hive/console
repository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
packages/fusion-runtime/src/utils.ts
⏰ Context from checks skipped due to timeout of 90000ms (25)
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: Unit / Bun
- GitHub Check: E2E / Bun on Ubuntu
- GitHub Check: Unit / Node v23
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: Leaks / Node v23
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: Leaks / Node v22
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: Bundle
- GitHub Check: Leaks / Node v20
- GitHub Check: Snapshot / snapshot
- GitHub Check: Build
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: Leaks / Node v18
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Benchmark / node / 10 items
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
packages/fusion-runtime/src/utils.ts (2)
16-16
: LGTM! Added required dependency.The import of
createBatchingExecutor
aligns with the PR's objective of moving batching functionality to the Gateway.
172-181
: LGTM! Well-structured parameter addition.The
batch
parameter is added with a sensible default value oftrue
, maintaining backward compatibility while enabling opt-out flexibility.
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/fusion-runtime/src/utils.ts (1)
243-248
: Consider adding error handling for batching executor.While the batching implementation is correct, it might benefit from error handling to gracefully handle cases where batching fails.
if (batch) { + try { executor = getBatchingExecutor( executionRequest.context || subgraphExecutorMap, executor, ); + } catch (error) { + logger?.warn('Failed to create batching executor, falling back to regular executor', { error }); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/fusion-runtime/src/utils.ts
(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`packages/**`: In this directory we keep all packages releva...
packages/**
: In this directory we keep all packages relevant to the gateway.In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using
yarn changeset
with a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminor
semver change. When a bug is fixed,patch
needs to be used. Themajor
bump is used for a PR that has breaking changes.
packages/fusion-runtime/src/utils.ts
`**`: For all PRs, we would like to verify that a Linear iss...
**
: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
console
project.
Use the global knowledge feature to search for PRs ingraphql-hive/console
repository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
packages/fusion-runtime/src/utils.ts
⏰ Context from checks skipped due to timeout of 90000ms (26)
- GitHub Check: Unit / Bun
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: Unit / Node v22
- GitHub Check: E2E / Bun on Ubuntu
- GitHub Check: Unit / Node v20
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: Leaks / Node v23
- GitHub Check: Snapshot / snapshot
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: Leaks / Node v22
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Leaks / Node v20
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: Leaks / Node v18
- GitHub Check: Bundle
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Build
- GitHub Check: Benchmark / node / 10 items
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
packages/fusion-runtime/src/utils.ts (3)
16-16
: LGTM: Added batching executor import.The import of
getBatchingExecutor
from@graphql-tools/batch-execute
aligns with the PR's objective of moving batching functionality to the Gateway.
172-172
: LGTM: Added optional batching parameter.The
batch
parameter with a default value oftrue
maintains backward compatibility while allowing explicit control over batching behavior.Also applies to: 181-181
1-713
: Add changeset file for the batching changes.According to the coding guidelines, we need a changeset file for changes to packages. Since this adds new functionality by moving batching to the Gateway, it should be a
minor
version bump.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if a changeset file exists for this PR fd -e md . .changeset/Length of output: 120
Changeset file for batching changes is present.
The.changeset
directory contains a file named.changeset/@graphql-mesh_fusion-runtime-605-dependencies.md
, which appears to cover the batching changes that were introduced in this PR. This changeset should trigger the required minor version bump according to our guidelines.🧰 Tools
🪛 Biome (1.9.4)
[error] 409-409: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 430-430: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 450-450: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
7fbe763
to
0172d63
Compare
@coderabbitai review again |
@ardatan I'll review the changes in the PR. ✅ Actions performedReview triggered.
|
Batching is now handled by the Gateway not Stitching so a future supergraph executor doesn't need to handle this on its own