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

Distributed query plan cache keys include the Router version number #6406

Merged
merged 7 commits into from
Dec 6, 2024
Merged

Distributed query plan cache keys include the Router version number #6406

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
21deaeb
Distributed query plan cache keys include the Router version number
SimonSapin Dec 5, 2024
0f7e2f0
Changelog
SimonSapin Dec 5, 2024
5e8acef
Wrong keys in tests
SimonSapin Dec 6, 2024
dff1c54
Swap changelog paragraphs as suggested in review
SimonSapin Dec 6, 2024
8a41d59
Merge remote-tracking branch 'origin/dev' into simon/router-version-i…
SimonSapin Dec 6, 2024
f79225d
Merge remote-tracking branch 'origin/dev' into simon/router-version-i…
SimonSapin Dec 6, 2024
18ad8a3
:,
SimonSapin Dec 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changesets/config_simon_router_version_in_cache_key.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
### Distributed query plan cache keys include the Router version number ([PR #6406](https://github.com/apollographql/router/pull/6406))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put this (though filename prefix) in the "Configuration" section of the changelog, but none of the options in xtask changeset create seemed like a great fit.

CHANGELOG.md has some > [!IMPORTANT] sections, is that done manually when editing for a release?

Copy link
Member

Choose a reason for hiding this comment

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

we do that manually in the release PR, since it has to go in the top of the changelog


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.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this doesn’t need to mention CACHE_KEY_VERSION v.s. FEDERATION_VERSION so I tried to pick a relevant level of detail.

Copy link
Member

Choose a reason for hiding this comment

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

I would put this paragraph first. The point is that almost every release has had a cache key change, which comes with the additional cache regeneration cost. If people read the first paragraph, they'll think this is more than what's currently happening, but it effectively isn't.


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
32 changes: 0 additions & 32 deletions apollo-router/build/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,37 +1,5 @@
use std::fs;
use std::path::PathBuf;

mod studio;

fn main() -> Result<(), Box<dyn std::error::Error>> {
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()
}
25 changes: 0 additions & 25 deletions apollo-router/src/query_planner/bridge_query_planner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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()
Expand Down
9 changes: 3 additions & 6 deletions apollo-router/src/query_planner/caching_query_planner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -632,10 +632,7 @@ pub(crate) struct CachingQueryKey {
pub(crate) config_mode: Arc<QueryHash>,
}

// 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 {
Expand All @@ -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,
)
}
}
Expand Down
30 changes: 24 additions & 6 deletions apollo-router/tests/integration/redis.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
}
Expand All @@ -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;
}
Expand All @@ -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;
Expand Down
Loading