-
Notifications
You must be signed in to change notification settings - Fork 244
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
refresh query plan expiration on GET #4604
Conversation
We want the query plan cache to act like a LRU, so if a TTL is set in its Redis configuration, we will reset the plan cache key's current expiration when reding it
This comment has been minimized.
This comment has been minimized.
CI performance tests
|
If possible. The "TTL reset" mechanism is helpful for this use-case imo. |
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.
Is there any way we can test this functionality by extending our existing redis integration tests?
also: config feature => changelog?
Probably yes, we can check a key's expiration by querying redis in the test |
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.
Missing the changelog but LGTM
there is now a test in f08b3ad. Integration tests will require Redis 7 from now on, to have access to the |
Fix apollographql#4473 We want the query plan cache to act like a LRU, so if a TTL is set in its Redis configuration, we will reset the plan cache key's current expiration when reading it. This is now part of the common redis configuration and is enabled by default. For entity caching, this option is disabled, as the cache will manage TTL directly Integration tests now require Redis 7. Otherwise you will get the error "ERR unknown command `EXPIRETIME`"
Fix #4473
We want the query plan cache to act like a LRU, so if a TTL is set in its Redis configuration, we will reset the plan cache key's current expiration when reding it
unclear to me here: should we instead make it an option on the common redis configuration type? We may want to control that behaviour for APQ and introspection tooThis is now part of the common redis configuration and defaults to true. For entity caching, this option is disabled, as the cache will manage TTL directlyTests now require REdis 7. Otherwise you will get the error "ERR unknown command
EXPIRETIME
"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. ↩