Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Query Plan Caching Improvements #4473

Closed
lleadbet opened this issue Jan 12, 2024 · 1 comment · Fixed by #4588 or #4604
Closed

Query Plan Caching Improvements #4473

lleadbet opened this issue Jan 12, 2024 · 1 comment · Fixed by #4588 or #4604
Assignees
@Geal
Labels
raised by user

Comments

@lleadbet
Copy link
Contributor

lleadbet commented Jan 12, 2024 •
edited

Describe the solution you'd like

The current functionality addresses most needs, but there are a couple of improvements that could be made to help make it more useful.

  • Include the federation version as part of the cache key
  • Refresh TTLs on recently accessed query plans (optionally)
  • Make the TTL a sane default vs. indefinite (in combination with the above)

The first allows for multiple different versions to use the same query plan cache, as different federation versions would generate different query plans in theory.

The second would ensure that stale entries are evicted, but hot query plan paths are retained as long as they are being used in the current schema. For distributed caches, this can minimize the amount of data being used in memory without manual pruning or having to take a performance hit every time the TTL expires.

Describe alternatives you've considered

n/a

Additional context

n/a
@Geal Geal mentioned this issue Feb 2, 2024
Merged
12 tasks
@Geal Geal self-assigned this Feb 7, 2024
This was referenced Feb 7, 2024
Merged
Merged
Geal added a commit that referenced this issue Mar 13, 2024
@Geal
refresh query plan expiration on GET (#4604)
65715f1 
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 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`"
@Geal
Copy link
Contributor

Geal commented Mar 14, 2024 •
edited

reopening because we're still missing #4588

@Geal Geal reopened this Mar 14, 2024
smyrick pushed a commit to smyrick/router that referenced this issue Mar 18, 2024
@Geal @smyrick
refresh query plan expiration on GET (apollographql#4604)
c3b9a71 
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`"
@abernix abernix mentioned this issue Mar 21, 2024
Merged
@Geal Geal closed this as completed in #4588 Apr 5, 2024
Geal added a commit that referenced this issue Apr 5, 2024
@Geal @lrlna
set a default TTL for query plans (#4588)
ebbe6ec 
Fix #4473

This sets a default TTL for query plan caches, because right now the
default is to store query plans indefinitely, which does not make sense
because they change with schema updates.
The PR is a bit verbose because to add that new default, we need to
create a redundant structure for redis configuration

Co-authored-by: Iryna Shestak <shestak.irina@gmail.com>
@o0Ignition0o o0Ignition0o mentioned this issue Apr 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Geal Geal

Labels
raised by user
Projects
None yet
Milestone
No milestone
2 participants
@Geal @lleadbet