Add the query planner options to the query plan cache key #5100

Geal · 2024-05-07T11:04:04Z

Some query plan options can result in incompatible generated plans if they are enabled or disabled, so we must take them into account in the cache key, otherwise a router might get a plan from the cache that does not match its configuration.
We cannot cover the entire apollo-federation config in the hash for now, this will have to be done as a follow up

Checklist

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

Exceptions

Note any exceptions here

Notes

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. ↩

router-perf · 2024-05-07T11:04:36Z

…nges when the query plan gets updated.

Rename tests to be in line with other tests in this file

BrynCooke · 2024-05-09T11:58:11Z

I added some testing for this which meant extending the integration test runner a little.

I'm not massively keen on the way that we obtain the config for the query planners. Once the rust query planner is hashable maybe we could consider implementing From<Configuration> for QueryPlannerConfig

abernix

I have reviewed some softer bits and requested changes around them on account of looking for answers to my questions, but I will leave the rest of the review to others for now. Happy to revisit.

.changesets/fix_geal_cache_key_qp_options.md

apollo-router/src/query_planner/caching_query_planner.rs

Co-authored-by: Jesse Rosenberger <git@jro.cc>

Skip redis tests on CI for anything other than x86_64

IvanGoncharov · 2024-05-09T17:49:42Z

apollo-router/src/query_planner/caching_query_planner.rs

-            "plan:{}:{}:{}:{}",
-            FEDERATION_VERSION, self.hash, operation, metadata,
+            "plan:{}:{}:{}:{}:{}",
+            CACHE_KEY_VERSION, FEDERATION_VERSION, self.hash, operation, metadata,


It's unrelated to this particular PR, but it took me a while to understand that operation is a hash of self.operation, which is actually an operation name.

Also, self.hash is hash of query and relevent parts of schema, but taking into account only operation matching operation_name.
At the same time, self.hash doesn't include a hash of "operation_name" itself

Also self.metadata is actually "authorization metadata".

P.S. Same as the previous comment, this is unrelated to this PR.
Just comments for others to also understand what is happening in this code.

I'd like to see this refactored and keys become self describing.

When/where does this change get itemized/made?

apollo-router/src/query_planner/caching_query_planner.rs

Geal · 2024-05-13T07:48:06Z

apollo-router/src/query_planner/caching_query_planner.rs

        hasher.update(&serde_json::to_vec(&self.sdl).expect("serialization should not fail"));
+        hasher.update([self.introspection as u8]);


why is introspection added here but not in the Hash implementation below?

Nice catch: 43ccd9d

I am wondering though why we are using a manual implementation of Hash rather than using derive? There seems to be nothing special about this structure that would warrant a custom impl.

I'd have to dig it up, but I think there was something in the key that could not implement Hash in the past

Once we figure that out, is there somewhere we could add a comment to make that more intuitive?

If we don't need a custom implementation now let's remove it and use derive. That way we don't have to worry about it in the future. If we do eventually need a custom version would need to comment and possibly fuzz test.

apollo-router/tests/common.rs

apollo-router/tests/integration/mod.rs

Co-authored-by: Jesse Rosenberger <git@jro.cc>

Geal · 2024-05-13T07:55:59Z

thank you all for the feedback, I will make another pass today or tomorrow to fix up the small issues. @BrynCooke do you want to see the hash prefxies in this PR or in a follow up? I plan to make another PR right after to fix other things, like removing the JSON usage when possible

BrynCooke · 2024-05-13T08:23:34Z

@Geal Happy for the key change to be a follow up.

Geal · 2024-05-13T12:26:50Z

@abernix can we merge this?

abernix · 2024-05-13T12:49:33Z

@Geal Does this completely supersede @fotoetienne's #5094? If so, curious if @xuorig / @fotoetienne could take a look.

My previous review is no longer relevant based on many new changes.

apollo-router/Cargo.toml

Geal · 2024-05-13T15:56:25Z

yes it supersedes that PR

fotoetienne · 2024-05-13T17:20:30Z

apollo-router/src/query_planner/caching_query_planner.rs

-            "plan:{}:{}:{}:{}",
-            FEDERATION_VERSION, self.hash, operation, metadata,
+            "plan:{}:{}:{}:{}:{}",
+            CACHE_KEY_VERSION, FEDERATION_VERSION, self.hash, operation, metadata,


Unrelated to this PR: Why are operation and metadata separate vs combined in the same hasher?

that was a way to see how many variants there can be in cache for a single query

Geal · 2024-05-14T07:47:23Z

follow up issue with the fixes we discussed: #5160

fotoetienne and others added 2 commits May 6, 2024 14:57

Add QueryPlannerConfig to QueryPlan key

25cb440

add the query planner config to the query plan cache key

5ca59ef

This comment has been minimized.

Sign in to view

apollo-bot2 assigned Geal May 7, 2024

Geal and others added 7 commits May 7, 2024 13:54

add a cache key version number

4912f56

Merge branch 'dev' into geal/cache-key-qp-options

abab512

Enable redis tests on OSX, fix test failure.

e7b3ad2

Add an integration test that checks that the query plan cache key cha…

10a6ae7

…nges when the query plan gets updated.

Add changeset

3db24b9

lint

cde9e63

Rename a bunch of files that changed due to the removal of mod test.

716901a

Rename tests to be in line with other tests in this file

BrynCooke requested review from abernix and IvanGoncharov May 9, 2024 11:55

Update changelog to reference original issue.

e9ffe66

abernix previously requested changes May 9, 2024

View reviewed changes

BrynCooke and others added 2 commits May 9, 2024 13:28

Update .changesets/fix_geal_cache_key_qp_options.md

605db97

Co-authored-by: Jesse Rosenberger <git@jro.cc>

Remove confusing changlog example

4968a88

BrynCooke requested a review from abernix May 9, 2024 13:40

bryn added 5 commits May 9, 2024 14:43

Add redis to non-amd ci

1d908d4

Add redis to non-amd ci

fbcb0d8

Remove redis from ci, we can't do this.

3c6ef29

Allow conditional compilation based on CI.

ebd15d0

Skip redis tests on CI for anything other than x86_64

Skip redis tests on CI for anything other than x86_64 && linux

c5375bc

IvanGoncharov reviewed May 9, 2024

View reviewed changes

apollo-router/src/query_planner/caching_query_planner.rs Show resolved Hide resolved

bryn added 3 commits May 10, 2024 10:10

Improve testing

bf0b938

Improve testing

b049a21

Make introspection part of the cache key

4bd73a8

Geal commented May 13, 2024

View reviewed changes

apollo-router/tests/common.rs Show resolved Hide resolved

Geal commented May 13, 2024

View reviewed changes

apollo-router/tests/integration/mod.rs Show resolved Hide resolved

Update apollo-router/src/query_planner/caching_query_planner.rs

ce20a8b

Co-authored-by: Jesse Rosenberger <git@jro.cc>

bryn and others added 2 commits May 13, 2024 09:31

Add introspection to Hash implementation.

43ccd9d

add an expiration to Redis configuration fixtures

17addbc

abernix reviewed May 13, 2024

View reviewed changes

apollo-router/Cargo.toml Show resolved Hide resolved

o0Ignition0o approved these changes May 13, 2024

View reviewed changes

bnjjj approved these changes May 13, 2024

View reviewed changes

fotoetienne approved these changes May 13, 2024

View reviewed changes

Geal merged commit f24ae3a into dev May 14, 2024
13 of 14 checks passed

Geal deleted the geal/cache-key-qp-options branch May 14, 2024 07:43

Geal mentioned this pull request May 14, 2024

Query plan cache key follow up #5160

Open

IvanGoncharov mentioned this pull request May 21, 2024

prep release: v1.47.0 #5208

Merged

Geal mentioned this pull request May 27, 2024

Fix cache key hashing algorithm #5255

Draft

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the query planner options to the query plan cache key #5100

Add the query planner options to the query plan cache key #5100

Geal commented May 7, 2024 •

edited

This comment has been minimized.

router-perf bot commented May 7, 2024

BrynCooke commented May 9, 2024

abernix left a comment

IvanGoncharov May 9, 2024

IvanGoncharov May 9, 2024

BrynCooke May 10, 2024

abernix May 13, 2024

Geal May 13, 2024

BrynCooke May 13, 2024

Geal May 13, 2024

abernix May 13, 2024

BrynCooke May 13, 2024

Geal commented May 13, 2024

BrynCooke commented May 13, 2024

Geal commented May 13, 2024

abernix commented May 13, 2024

Geal commented May 13, 2024

fotoetienne May 13, 2024

Geal May 14, 2024

Geal commented May 14, 2024

		hasher.update(&serde_json::to_vec(&self.sdl).expect("serialization should not fail"));
		hasher.update([self.introspection as u8]);

Add the query planner options to the query plan cache key #5100

Add the query planner options to the query plan cache key #5100

Conversation

Geal commented May 7, 2024 • edited

Footnotes

This comment has been minimized.

router-perf bot commented May 7, 2024

BrynCooke commented May 9, 2024

abernix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Geal commented May 13, 2024

BrynCooke commented May 13, 2024

Geal commented May 13, 2024

abernix commented May 13, 2024

Geal commented May 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Geal commented May 14, 2024

Geal commented May 7, 2024 •

edited