From a201cf5ae6f38377069e54b7e379bb3e81d61159 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Fri, 6 Dec 2024 18:36:09 +0100 Subject: [PATCH] Distributed query plan cache keys include the Router version number (#6406) --- ...onfig_simon_router_version_in_cache_key.md | 7 ++++ apollo-router/build/main.rs | 32 ------------------- .../src/query_planner/bridge_query_planner.rs | 25 --------------- .../query_planner/caching_query_planner.rs | 9 ++---- apollo-router/tests/integration/redis.rs | 30 +++++++++++++---- 5 files changed, 34 insertions(+), 69 deletions(-) create mode 100644 .changesets/config_simon_router_version_in_cache_key.md diff --git a/.changesets/config_simon_router_version_in_cache_key.md b/.changesets/config_simon_router_version_in_cache_key.md new file mode 100644 index 0000000000..ce8b9a8690 --- /dev/null +++ b/.changesets/config_simon_router_version_in_cache_key.md @@ -0,0 +1,7 @@ +### Distributed query plan cache keys include the Router version number ([PR #6406](https://github.com/apollographql/router/pull/6406)) + +More often than not, an Apollo Router release may contain changes that affect what query plans are generated or how they’re represented. To avoid using outdated entries from distributed cache, the cache key includes a counter that was manually incremented with relevant data structure or algorithm changes. Instead the cache key now includes the Router version number, so that different versions will always use separate cache entries. + +If you have enabled [Distributed query plan caching](https://www.apollographql.com/docs/router/configuration/distributed-caching/#distributed-query-plan-caching), starting with this release and going forward you should anticipate additional cache regeneration cost when updating between any Router versions. + +By [@SimonSapin](https://github.com/SimonSapin) in https://github.com/apollographql/router/pull/6406 diff --git a/apollo-router/build/main.rs b/apollo-router/build/main.rs index 763d894df0..b323c668bb 100644 --- a/apollo-router/build/main.rs +++ b/apollo-router/build/main.rs @@ -1,37 +1,5 @@ -use std::fs; -use std::path::PathBuf; - mod studio; fn main() -> Result<(), Box> { - let cargo_manifest: serde_json::Value = basic_toml::from_str( - &fs::read_to_string(PathBuf::from(&env!("CARGO_MANIFEST_DIR")).join("Cargo.toml")) - .expect("could not read Cargo.toml"), - ) - .expect("could not parse Cargo.toml"); - - let router_bridge = cargo_manifest - .get("dependencies") - .expect("Cargo.toml does not contain dependencies") - .as_object() - .expect("Cargo.toml dependencies key is not an object") - .get("router-bridge") - .expect("Cargo.toml dependencies does not have an entry for router-bridge"); - let router_bridge_version = router_bridge - .as_str() - .or_else(|| { - router_bridge - .as_object() - .and_then(|o| o.get("version")) - .and_then(|version| version.as_str()) - }) - .expect("router-bridge does not have a version"); - - let mut it = router_bridge_version.split('+'); - let _ = it.next(); - let fed_version = it.next().expect("invalid router-bridge version format"); - - println!("cargo:rustc-env=FEDERATION_VERSION={fed_version}"); - studio::main() } diff --git a/apollo-router/src/query_planner/bridge_query_planner.rs b/apollo-router/src/query_planner/bridge_query_planner.rs index 4c820299f5..b60d62f3f5 100644 --- a/apollo-router/src/query_planner/bridge_query_planner.rs +++ b/apollo-router/src/query_planner/bridge_query_planner.rs @@ -843,9 +843,6 @@ pub(crate) fn metric_rust_qp_init(init_error_kind: Option<&'static str>) { #[cfg(test)] mod tests { - use std::fs; - use std::path::PathBuf; - use serde_json::json; use test_log::test; use tower::Service; @@ -1389,28 +1386,6 @@ mod tests { .await } - #[test] - fn router_bridge_dependency_is_pinned() { - let cargo_manifest: serde_json::Value = basic_toml::from_str( - &fs::read_to_string(PathBuf::from(&env!("CARGO_MANIFEST_DIR")).join("Cargo.toml")) - .expect("could not read Cargo.toml"), - ) - .expect("could not parse Cargo.toml"); - let router_bridge_version = cargo_manifest - .get("dependencies") - .expect("Cargo.toml does not contain dependencies") - .as_object() - .expect("Cargo.toml dependencies key is not an object") - .get("router-bridge") - .expect("Cargo.toml dependencies does not have an entry for router-bridge") - .as_str() - .unwrap_or_default(); - assert!( - router_bridge_version.contains('='), - "router-bridge in Cargo.toml is not pinned with a '=' prefix" - ); - } - #[tokio::test] async fn test_both_mode() { let mut harness = crate::TestHarness::builder() diff --git a/apollo-router/src/query_planner/caching_query_planner.rs b/apollo-router/src/query_planner/caching_query_planner.rs index 209adcc856..7b67bf30ec 100644 --- a/apollo-router/src/query_planner/caching_query_planner.rs +++ b/apollo-router/src/query_planner/caching_query_planner.rs @@ -632,10 +632,7 @@ pub(crate) struct CachingQueryKey { pub(crate) config_mode: Arc, } -// Update this key every time the cache key or the query plan format has to change. -// When changed it MUST BE CALLED OUT PROMINENTLY IN THE CHANGELOG. -const CACHE_KEY_VERSION: usize = 1; -const FEDERATION_VERSION: &str = std::env!("FEDERATION_VERSION"); +const ROUTER_VERSION: &str = env!("CARGO_PKG_VERSION"); impl std::fmt::Display for CachingQueryKey { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { @@ -654,8 +651,8 @@ impl std::fmt::Display for CachingQueryKey { write!( f, - "plan:cache:{}:federation:{}:{}:opname:{}:metadata:{}", - CACHE_KEY_VERSION, FEDERATION_VERSION, self.hash, operation, metadata, + "plan:router:{}:{}:opname:{}:metadata:{}", + ROUTER_VERSION, self.hash, operation, metadata, ) } } diff --git a/apollo-router/tests/integration/redis.rs b/apollo-router/tests/integration/redis.rs index 5f41d8e546..d7428ab4e4 100644 --- a/apollo-router/tests/integration/redis.rs +++ b/apollo-router/tests/integration/redis.rs @@ -51,7 +51,10 @@ async fn query_planner_cache() -> Result<(), BoxError> { } // If this test fails and the cache key format changed you'll need to update the key here. // Look at the top of the file for instructions on getting the new cache key. - let known_cache_key = "plan:cache:1:federation:v2.9.3:8c0b4bfb4630635c2b5748c260d686ddb301d164e5818c63d6d9d77e13631676:opname:3973e022e93220f9212c18d0d0c543ae7c309e46640da93a4a0314de999f5112:metadata:81b9c296d8ebc8adf4575b83a8d296621fd76de6d12d8c91f4552eda02d1dd9c"; + let known_cache_key = &format!( + "plan:router:{}:8c0b4bfb4630635c2b5748c260d686ddb301d164e5818c63d6d9d77e13631676:opname:3973e022e93220f9212c18d0d0c543ae7c309e46640da93a4a0314de999f5112:metadata:81b9c296d8ebc8adf4575b83a8d296621fd76de6d12d8c91f4552eda02d1dd9c", + env!("CARGO_PKG_VERSION") + ); let config = RedisConfig::from_url("redis://127.0.0.1:6379").unwrap(); let client = RedisClient::new(config, None, None, None); @@ -988,7 +991,10 @@ async fn query_planner_redis_update_query_fragments() { test_redis_query_plan_config_update( // This configuration turns the fragment generation option *off*. include_str!("fixtures/query_planner_redis_config_update_query_fragments.router.yaml"), - "plan:cache:1:federation:v2.9.3:5938623f2155169070684a48be1e0b8468d0f2c662b5527a2247f683173f7d05:opname:3973e022e93220f9212c18d0d0c543ae7c309e46640da93a4a0314de999f5112:metadata:a55ce3338ce6d5b78566be89cc6a7ad3fe8a7eeb38229d14ddf647edef84e545", + &format!( + "plan:router:{}:5938623f2155169070684a48be1e0b8468d0f2c662b5527a2247f683173f7d05:opname:3973e022e93220f9212c18d0d0c543ae7c309e46640da93a4a0314de999f5112:metadata:a55ce3338ce6d5b78566be89cc6a7ad3fe8a7eeb38229d14ddf647edef84e545", + env!("CARGO_PKG_VERSION") + ), ) .await; } @@ -1018,7 +1024,10 @@ async fn query_planner_redis_update_defer() { // test just passes locally. test_redis_query_plan_config_update( include_str!("fixtures/query_planner_redis_config_update_defer.router.yaml"), - "plan:cache:1:federation:v2.9.3:5938623f2155169070684a48be1e0b8468d0f2c662b5527a2247f683173f7d05:opname:3973e022e93220f9212c18d0d0c543ae7c309e46640da93a4a0314de999f5112:metadata:0497501d3d01d05ad142938e7b8d8e7ea13e648aabbbedb47f6291ca8b3e536d", + &format!( + "plan:router:{}:5938623f2155169070684a48be1e0b8468d0f2c662b5527a2247f683173f7d05:opname:3973e022e93220f9212c18d0d0c543ae7c309e46640da93a4a0314de999f5112:metadata:0497501d3d01d05ad142938e7b8d8e7ea13e648aabbbedb47f6291ca8b3e536d", + env!("CARGO_PKG_VERSION") + ), ) .await; } @@ -1040,7 +1049,10 @@ async fn query_planner_redis_update_type_conditional_fetching() { include_str!( "fixtures/query_planner_redis_config_update_type_conditional_fetching.router.yaml" ), - "plan:cache:1:federation:v2.9.3:5938623f2155169070684a48be1e0b8468d0f2c662b5527a2247f683173f7d05:opname:3973e022e93220f9212c18d0d0c543ae7c309e46640da93a4a0314de999f5112:metadata:662f5041882b3f621aeb7bad8e18818173eb077dc4343e16f3a34d2b6b6e4e59", + &format!( + "plan:router:{}:5938623f2155169070684a48be1e0b8468d0f2c662b5527a2247f683173f7d05:opname:3973e022e93220f9212c18d0d0c543ae7c309e46640da93a4a0314de999f5112:metadata:662f5041882b3f621aeb7bad8e18818173eb077dc4343e16f3a34d2b6b6e4e59", + env!("CARGO_PKG_VERSION") + ), ) .await; } @@ -1063,7 +1075,10 @@ async fn query_planner_redis_update_reuse_query_fragments() { include_str!( "fixtures/query_planner_redis_config_update_reuse_query_fragments.router.yaml" ), - "plan:cache:1:federation:v2.9.3:5938623f2155169070684a48be1e0b8468d0f2c662b5527a2247f683173f7d05:opname:3973e022e93220f9212c18d0d0c543ae7c309e46640da93a4a0314de999f5112:metadata:9af18c8afd568c197050fc1a60c52a8c98656f1775016110516fabfbedc135fe", + &format!( + "plan:router:{}:5938623f2155169070684a48be1e0b8468d0f2c662b5527a2247f683173f7d05:opname:3973e022e93220f9212c18d0d0c543ae7c309e46640da93a4a0314de999f5112:metadata:9af18c8afd568c197050fc1a60c52a8c98656f1775016110516fabfbedc135fe", + env!("CARGO_PKG_VERSION") + ), ) .await; } @@ -1088,7 +1103,10 @@ async fn test_redis_query_plan_config_update(updated_config: &str, new_cache_key router.clear_redis_cache().await; // If the tests above are failing, this is the key that needs to be changed first. - let starting_key = "plan:cache:1:federation:v2.9.3:5938623f2155169070684a48be1e0b8468d0f2c662b5527a2247f683173f7d05:opname:3973e022e93220f9212c18d0d0c543ae7c309e46640da93a4a0314de999f5112:metadata:81b9c296d8ebc8adf4575b83a8d296621fd76de6d12d8c91f4552eda02d1dd9c"; + let starting_key = &format!( + "plan:router:{}:5938623f2155169070684a48be1e0b8468d0f2c662b5527a2247f683173f7d05:opname:3973e022e93220f9212c18d0d0c543ae7c309e46640da93a4a0314de999f5112:metadata:81b9c296d8ebc8adf4575b83a8d296621fd76de6d12d8c91f4552eda02d1dd9c", + env!("CARGO_PKG_VERSION") + ); assert_ne!(starting_key, new_cache_key, "starting_key (cache key for the initial config) and new_cache_key (cache key with the updated config) should not be equal. This either means that the cache key is not being generated correctly, or that the test is not actually checking the updated key."); router.execute_default_query().await;