-
Notifications
You must be signed in to change notification settings - Fork 272
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
Enable generate_query_fragments
by default
#6013
Open
lrlna
wants to merge
12
commits into
dev
Choose a base branch
from
lrlna/enable-generate-fragments
base: dev
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
be83e30
Enable `generate_query_fragments` by default
lrlna ee0975d
update config snapshot
lrlna cd3c249
Update docs/source/configuration/overview.mdx
lrlna 1daae5d
use unwrap_or_else for generate_query_fragments defaults
lrlna 51e89cb
update redis cache keys
lrlna 6f43f20
new changelog
goto-bus-stop a6035eb
Merge branch 'dev' into lrlna/enable-generate-fragments
goto-bus-stop e807ff6
explain experimental/legacy state of fragment reuse in docs; do not s…
goto-bus-stop f1f45a7
keep links to the old header name working
goto-bus-stop 0c40214
Update mocks: migrate some to fragment generation, disable fragment g…
goto-bus-stop 544ce20
Merge branch 'dev' into lrlna/enable-generate-fragments
goto-bus-stop f5e1898
Update more mocks to use frag generation
goto-bus-stop File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
21 changes: 21 additions & 0 deletions
21
.changesets/feat_lrlna_enable_generate_query_fragments_by_default.md
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
### Enable query fragment generation by default | ||
|
||
The router previously had `experimental_reuse_query_fragments` enabled by | ||
default when trying to optimize fragments before sending operations to subgraphs. | ||
While on occasion this algorithm can be more performant, we found that in vast | ||
majority of cases the query planner can be just as performant using | ||
`generate_query_fragments` query plan option, which also significantly reduces | ||
query size being sent to subgraphs. While the two options will produce correct | ||
responses, the queries produced internally by the query planner will differ. | ||
|
||
This change enables `generate_query_fragments` by default, while disabling | ||
`experimental_reuse_query_fragments`. You can change this behavior with the | ||
following options: | ||
|
||
```yaml | ||
supergraph: | ||
generate_query_fragments: false | ||
experimental_reuse_query_fragments: true | ||
``` | ||
|
||
By [@lrlna](https://github.com/lrlna) in https://github.com/apollographql/router/pull/6013 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
this fails our own
default
tests:The only way for this config to have its own
Default
implementation is to be defined as an enum/struct, which feels wayyy too much for something like this. Any other ideas on setting up a default in a single place for this config?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.
Remove
#[serde(default = "default_generate_query_fragments")]
and replace two instances ofgenerate_query_fragments.unwrap_or_default()
in this file withgenerate_query_fragments.unwrap_or_else(default_generate_query_fragments)
Since the struct already has
#[serde(default)]
serde will rely on the existingimpl Default for Supergraph
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.
i felt that using
unwrap_or_else
hid away the default definition. but i guess since we've got a doc comment withDefault: true
it's maybe a bit more clear.