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

initial stab at making the router prewarm on pqs #5340

Merged
merged 10 commits into from
Jun 11, 2024
Merged

initial stab at making the router prewarm on pqs #5340

merged 10 commits into from
Jun 11, 2024

Conversation

lleadbet
Copy link
Contributor

@lleadbet lleadbet commented Jun 4, 2024 •
edited
Loading

As #5334 outlined, the existing query plan warmup mechanism only warmed up the cache upon reload of either the schema or (if using dev mode/hot-reload) the config.

This PR allows the warmup function to occur on startup as well, enabling persisted queries to be used to warm up prior to serving traffic, helping reduce latency introduced by the query planner.

Fixes #5334

Checklist

Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.

  • Changes are compatible[^1]
  • Documentation[^2] completed
  • Performance impact assessed and acceptable
  • Tests added and passing[^3]
    • Unit Tests
    • Integration Tests
    • Manual Tests

Exceptions

The existing setup had no tests, so unsure how/where tests should be written for this.

@lleadbet lleadbet requested review from a team as code owners June 4, 2024 19:44
@router-perf
Copy link

router-perf bot commented Jun 4, 2024

CI performance tests

  • step - Basic stress test that steps up the number of users over time
  • reload - Reload test over a long period of time at a constant rate of users
  • step-with-prometheus - A copy of the step test with the Prometheus metrics exporter enabled
  • events_without_dedup_callback - Stress test for events with a lot of users and deduplication DISABLED using callback mode
  • events - Stress test for events with a lot of users and deduplication ENABLED
  • events_without_dedup - Stress test for events with a lot of users and deduplication DISABLED
  • events_big_cap_high_rate_callback - Stress test for events with a lot of users, deduplication enabled and high rate event with a big queue capacity using callback mode
  • xlarge-request - Stress test with 10 MB request payload
  • const - Basic stress test that runs with a constant number of users
  • events_big_cap_high_rate - Stress test for events with a lot of users, deduplication enabled and high rate event with a big queue capacity
  • step-jemalloc-tuning - Clone of the basic stress test for jemalloc tuning
  • xxlarge-request - Stress test with 100 MB request payload
  • demand-control-uninstrumented - A copy of the step test, but with demand control monitoring enabled
  • no-graphos - Basic stress test, no GraphOS.
  • events_callback - Stress test for events with a lot of users and deduplication ENABLED in callback mode
  • large-request - Stress test with a 1 MB request payload
  • demand-control-instrumented - A copy of the step test, but with demand control monitoring and metrics enabled

@Geal Geal requested a review from a team June 6, 2024 16:23
@abernix abernix requested a review from a team June 6, 2024 16:56
abernix
abernix reviewed Jun 7, 2024
View reviewed changes
Copy link
Member

@abernix abernix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we were to mark this feature in particular as experimental for the initial drop, how would we do that? Would we do it with configuration? In other words, how do we make it so we DO NOT use pre-warming from the PQ manifest unless it's explicitly enabled?

@lleadbet
Copy link
Contributor Author

lleadbet commented Jun 7, 2024

If we were to mark this feature in particular as experimental for the initial drop, how would we do that? Would we do it with configuration? In other words, how do we make it so we DO NOT use pre-warming from the PQ manifest unless it's explicitly enabled?

We could just add a config option under the query planning chunk if that's preferable.

I believe we've stated with others before that this is expected behavior, but if we'd like to keep it safe, we can certainly make it opt-in.

@abernix
Copy link
Member

abernix commented Jun 7, 2024

Ok, understood. It was really more about making it safe and validating it with a few customers first, but if we can claim it's safe, I'm comfortable.

@Geal
Copy link
Contributor

Geal commented Jun 10, 2024

I'd be ok with an option here too

@lleadbet lleadbet requested a review from a team as a code owner June 10, 2024 14:12
@lleadbet
Copy link
Contributor Author

Updated to use a new setting @Geal / @abernix. Just one note: I stuck it under the PQ settings since it seemed related to that instead, as well as it only applying to warming up on startup only, as we don't want to break existing setups.

bnjjj
bnjjj approved these changes Jun 10, 2024
View reviewed changes
docs/source/configuration/persisted-queries.mdx Outdated Show resolved Hide resolved
@lleadbet @bnjjj
Update docs/source/configuration/persisted-queries.mdx
ff7e1bd 
Co-authored-by: Coenen Benjamin <benjamin.coenen@hotmail.com>
@Geal Geal merged commit bcd9812 into apollographql:dev Jun 11, 2024
12 of 13 checks passed
@lleadbet lleadbet deleted the feat/pq-warmup branch June 11, 2024 16:35
@lrlna lrlna mentioned this pull request Jun 18, 2024
Merged
glasser added a commit that referenced this pull request Sep 10, 2024
@glasser
Allow disabling PQ-based QP cache rewarm when reloading schemas
37f34b1 
In #3829 we taught router to use the PQ list to prewarm the query
planner cache when reloading its schema.

In #5340 we gave it the experimental option to do the same thing at
startup.

Now we are allowing you to turn off the on-reload prewarm.

Because the PQ list does not have metadata to tell the Router which PQs
have been used recently, if you have a large PQ list then the on-reload
prewarm can slow down schema updates noticeably, and it may not provide
much value over prewarming based on the contents of the in-memory QP
cache itself.

We keep the same defaults, but let you turn off the on-reload prewarm
with:

```
persisted_queries:
  experimental_prewarm_query_plan_cache:
    on_reload: false
```

Previously, `persisted_queries.experimental_prewarm_query_plan_cache`
was a boolean; a migration moves this boolean to
`persisted_queries.experimental_prewarm_query_plan_cache.on_startup`.
(`on_startup` defaults to false and `on_reload` defaults to true.)

This migration uses a `type: change` action. No such actions existed in
the codebase and the implementation appeared to be buggy: there is no
top-level `==` operator (or top-level operators at all) in JSONPath.
This PR fixes the implementation of `type: change` to use the
`[?(@.x == y)]` filter syntax which does appear to work (as tested by
the migration tests for the new migration).
@glasser glasser mentioned this pull request Sep 10, 2024
Merged
6 tasks
glasser added a commit that referenced this pull request Sep 10, 2024
@glasser
Allow disabling PQ-based QP cache rewarm when reloading schemas
9ea2b7c 
In #3829 we taught router to use the PQ list to prewarm the query
planner cache when reloading its schema.

In #5340 we gave it the experimental option to do the same thing at
startup.

Now we are allowing you to turn off the on-reload prewarm.

Because the PQ list does not have metadata to tell the Router which PQs
have been used recently, if you have a large PQ list then the on-reload
prewarm can slow down schema updates noticeably, and it may not provide
much value over prewarming based on the contents of the in-memory QP
cache itself.

We keep the same defaults, but let you turn off the on-reload prewarm
with:

```
persisted_queries:
  experimental_prewarm_query_plan_cache:
    on_reload: false
```

Previously, `persisted_queries.experimental_prewarm_query_plan_cache`
was a boolean; a migration moves this boolean to
`persisted_queries.experimental_prewarm_query_plan_cache.on_startup`.
(`on_startup` defaults to false and `on_reload` defaults to true.)

This migration uses a `type: change` action. No such actions existed in
the codebase and the implementation appeared to be buggy: there is no
top-level `==` operator (or top-level operators at all) in JSONPath.
This PR fixes the implementation of `type: change` to use the
`[?(@.x == y)]` filter syntax which does appear to work (as tested by
the migration tests for the new migration).
glasser added a commit that referenced this pull request Sep 10, 2024
@glasser
Allow disabling PQ-based QP cache rewarm when reloading schemas
d279aab 
In #3829 we taught router to use the PQ list to prewarm the query
planner cache when reloading its schema.

In #5340 we gave it the experimental option to do the same thing at
startup.

Now we are allowing you to turn off the on-reload prewarm.

Because the PQ list does not have metadata to tell the Router which PQs
have been used recently, if you have a large PQ list then the on-reload
prewarm can slow down schema updates noticeably, and it may not provide
much value over prewarming based on the contents of the in-memory QP
cache itself.

We keep the same defaults, but let you turn off the on-reload prewarm
with:

```
persisted_queries:
  experimental_prewarm_query_plan_cache:
    on_reload: false
```

Previously, `persisted_queries.experimental_prewarm_query_plan_cache`
was a boolean; a migration moves this boolean to
`persisted_queries.experimental_prewarm_query_plan_cache.on_startup`.
(`on_startup` defaults to false and `on_reload` defaults to true.)

This migration uses a `type: change` action. No such actions existed in
the codebase and the implementation appeared to be buggy: there is no
top-level `==` operator (or top-level operators at all) in JSONPath.
This PR fixes the implementation of `type: change` to use the
`[?(@.x == y)]` filter syntax which does appear to work (as tested by
the migration tests for the new migration).
glasser added a commit that referenced this pull request Sep 10, 2024
@glasser
Allow disabling PQ-based QP cache rewarm when reloading schemas
f175271 
In #3829 we taught router to use the PQ list to prewarm the query
planner cache when reloading its schema.

In #5340 we gave it the experimental option to do the same thing at
startup.

Now we are allowing you to turn off the on-reload prewarm.

Because the PQ list does not have metadata to tell the Router which PQs
have been used recently, if you have a large PQ list then the on-reload
prewarm can slow down schema updates noticeably, and it may not provide
much value over prewarming based on the contents of the in-memory QP
cache itself.

We keep the same defaults, but let you turn off the on-reload prewarm
with:

```
persisted_queries:
  experimental_prewarm_query_plan_cache:
    on_reload: false
```

Previously, `persisted_queries.experimental_prewarm_query_plan_cache`
was a boolean; a migration moves this boolean to
`persisted_queries.experimental_prewarm_query_plan_cache.on_startup`.
(`on_startup` defaults to false and `on_reload` defaults to true.)

This migration uses a `type: change` action. No such actions existed in
the codebase and the implementation appeared to be buggy: there is no
top-level `==` operator (or top-level operators at all) in JSONPath.
This PR fixes the implementation of `type: change` to use the
`[?(@.x == y)]` filter syntax which does appear to work (as tested by
the migration tests for the new migration).

This also renames a migration file added in #5957 that used a
non-standard filename.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abernix abernix abernix left review comments

@bnjjj bnjjj bnjjj approved these changes

@BrynCooke BrynCooke BrynCooke approved these changes

@Geal Geal Geal approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Warm up query plan cache at boot using persisted queries
5 participants
@lleadbet @abernix @Geal @BrynCooke @bnjjj