-
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
Drop experimental reuse fragments optimization from RS QP #6354
Conversation
✅ Docs Preview ReadyNo new or changed pages found. |
CI performance tests
|
followup: we'll need to update the docs at https://www.apollographql.com/docs/graphos/reference/router/configuration#fragment-generation-and-reuse |
We should also make sure that people aren't opting back into the old algorithm en masse in 1.58.0. But this is a very exciting diff 😁 |
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 PR also needs to update docs for fragment generation and reuse.
Otherwise really exciting to get rid of all the dead code!!
410e6fe
to
cab4987
Compare
Drop support for the experimental reuse fragment query optimization. This implementation was very slow and buggy due to its complexity. Auto generation of fragments is much simpler algorithm that in most cases produces better results (and much faster).
5e7a2a3
to
946998e
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.
I am so sorry, I read a tracing::warn message you wrote and realised this is going to need a config migration. For the time being it can be a log
, and for the next release we can do a delete
.
if self | ||
.supergraph | ||
.reuse_query_fragments | ||
.is_some_and(|flag| flag) | ||
{ | ||
// warn the user that reuse query fragments is unsupported for RS QP | ||
tracing::warn!("'experimental_reuse_query_fragments' is not supported by the Rust QP and this configuration option will be removed in the next release. Use 'generate_query_fragments' instead."); | ||
} |
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.
Hm, we are going to need a migration for this config going away, I just realised. At this point it can be a log
rather than a delete
, but it should go through our regular "migration" process rather than adhoc, I think.
...uter/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap
Outdated
Show resolved
Hide resolved
Co-authored-by: Iryna Shestak <shestak.irina@gmail.com>
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.
Thank you!
Drop support for the experimental reuse fragment query optimization from Rust QP. This implementation was not only very slow but also very buggy due to its complexity.
experimental_reuse_query_fragments
option can be applied for the JS QP only.Auto generation of fragments is a much simpler (and faster) algorithm that in most cases produces better results. Fragment auto generation is the default optimization since v1.58 release.
Checklist
Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.
Exceptions
Note any exceptions here
Notes
Footnotes
It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩