Enable generate_query_fragments by default
#6013
Conversation
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 ```
✅ Docs Preview ReadyNo new or changed pages found. |
…how disabling fragment generation entirely (nobody should need to)
once #6013 lands, we'll need the code in connectors that converts fetch node operations to http calls to understand named fragments.
- Disable fragment generation on the tests that use mocks. - Update the expected cache key on the other tests. - Change the "update_generate_query_fragments" test to turn the option *off*, so it actually tests an update from the default *on*.
|
Oh there's been some new tests introduced in the mean time that are failing now 😭 |
| @@ -218,6 +218,10 @@ mod tests { | |||
| build_mock_supergraph(serde_json::json! {{ | |||
| "plugins": { | |||
| "experimental.expose_query_plan": true | |||
| }, | |||
| "supergraph": { | |||
| // TODO(@goto-bus-stop): need to update the mocks and remove this, #6013 | |||
There was a problem hiding this comment.
should this be linking to a new issue (6013 is the existing PR#)?
There was a problem hiding this comment.
The work would likely be done as part of removing reuse_query_fragments, I linked the PR as a place where you can find context
| @@ -7,5 +7,5 @@ supergraph: | |||
| urls: | |||
| - redis://localhost:6379 | |||
| ttl: 10s | |||
| generate_query_fragments: true | |||
| generate_query_fragments: false | |||
There was a problem hiding this comment.
was this intentional switch to false? guess the test is to verify non-default?
There was a problem hiding this comment.
Yes, it's to make sure the cache key updates when the config is flipped, so this has to use the non-default value
There was a problem hiding this comment.
Shouldn't we also flip the defaults in the query planner itself? i.e. here -> https://github.com/apollographql/router/blob/dev/apollo-federation/src/query_plan/query_planner.rs#L110
goto-bus-stop
requested review from
sachindshinde,
TylerBloom and
duckki
as code owners
|
That also requires updating many tests. I've instead disabled both optimizations by default in the planner. We should change the default inside apollo-federation after some improvements to the generation algorithm I think. |
| generate_query_fragments: false, | ||
| subgraph_graphql_validation: false, | ||
| incremental_delivery: Default::default(), | ||
| debug: Default::default(), | ||
| type_conditioned_fetching: Default::default(), | ||
| type_conditioned_fetching: false, |
There was a problem hiding this comment.
nit: I believe Default::default() for boolean is false. Should we update other defaults to explicit false as well (for readability)?
There was a problem hiding this comment.
Indeed. I wanted to be explicit about this. incremental_delivery and debug are not booleans but structs, so i kept them as Default::default. We could consider manually writing out Default impls for those types.
The router previously had
experimental_reuse_query_fragmentsenabled 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 usinggenerate_query_fragmentsquery 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_fragmentsby default, while disablingexperimental_reuse_query_fragments. You can change this behavior with the following options: