From f91548de98b382b43ea9d212a4f1985d5dea6821 Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Fri, 25 Nov 2022 15:18:49 +0100 Subject: [PATCH 1/3] request retries are deactivated by default on mutations --- apollo-router/src/plugins/traffic_shaping/mod.rs | 12 ++++++++++-- .../src/plugins/traffic_shaping/retry.rs | 16 +++++++++++++--- docs/source/configuration/traffic-shaping.mdx | 1 + 3 files changed, 24 insertions(+), 5 deletions(-) diff --git a/apollo-router/src/plugins/traffic_shaping/mod.rs b/apollo-router/src/plugins/traffic_shaping/mod.rs index be952aaaf1..e87efa3410 100644 --- a/apollo-router/src/plugins/traffic_shaping/mod.rs +++ b/apollo-router/src/plugins/traffic_shaping/mod.rs @@ -110,6 +110,9 @@ struct RetryConfig { /// retries allowed for via min_per_sec. Must be between 0 and 1000, default value /// is 0.2 retry_percent: Option, + /// allows request retries on mutations. This should only be activated if mutations + /// are idempotent. Disabled by default + retryable_mutations: Option, } impl Merge for RetryConfig { @@ -120,6 +123,7 @@ impl Merge for RetryConfig { ttl: self.ttl.or(fallback.ttl), min_per_sec: self.min_per_sec.or(fallback.min_per_sec), retry_percent: self.retry_percent.or(fallback.retry_percent), + retryable_mutations: self.retryable_mutations.or(fallback.retryable_mutations), }, } } @@ -318,8 +322,12 @@ impl TrafficShaping { }); let retry = config.experimental_retry.as_ref().map(|config| { - let retry_policy = - RetryPolicy::new(config.ttl, config.min_per_sec, config.retry_percent); + let retry_policy = RetryPolicy::new( + config.ttl, + config.min_per_sec, + config.retry_percent, + config.retryable_mutations, + ); tower::retry::RetryLayer::new(retry_policy) }); diff --git a/apollo-router/src/plugins/traffic_shaping/retry.rs b/apollo-router/src/plugins/traffic_shaping/retry.rs index 6138581c21..c212a2b06e 100644 --- a/apollo-router/src/plugins/traffic_shaping/retry.rs +++ b/apollo-router/src/plugins/traffic_shaping/retry.rs @@ -5,9 +5,13 @@ use std::time::Duration; use tower::retry::budget::Budget; use tower::retry::Policy; +use crate::query_planner::OperationKind; +use crate::services::subgraph; + #[derive(Clone, Default)] pub(crate) struct RetryPolicy { budget: Arc, + retryable_mutations: bool, } impl RetryPolicy { @@ -15,6 +19,7 @@ impl RetryPolicy { duration: Option, min_per_sec: Option, retry_percent: Option, + retryable_mutations: Option, ) -> Self { Self { budget: Arc::new(Budget::new( @@ -22,14 +27,15 @@ impl RetryPolicy { min_per_sec.unwrap_or(10), retry_percent.unwrap_or(0.2), )), + retryable_mutations: retryable_mutations.unwrap_or(false), } } } -impl Policy for RetryPolicy { +impl Policy for RetryPolicy { type Future = future::Ready; - fn retry(&self, _req: &Req, result: Result<&Res, &E>) -> Option { + fn retry(&self, req: &subgraph::Request, result: Result<&Res, &E>) -> Option { match result { Ok(_) => { // Treat all `Response`s as success, @@ -38,6 +44,10 @@ impl Policy for RetryPolicy { None } Err(_e) => { + if req.operation_kind == OperationKind::Mutation && !self.retryable_mutations { + return None; + } + let withdrew = self.budget.withdraw(); if withdrew.is_err() { return None; @@ -48,7 +58,7 @@ impl Policy for RetryPolicy { } } - fn clone_request(&self, req: &Req) -> Option { + fn clone_request(&self, req: &subgraph::Request) -> Option { Some(req.clone()) } } diff --git a/docs/source/configuration/traffic-shaping.mdx b/docs/source/configuration/traffic-shaping.mdx index c61f48e698..001790e7f3 100644 --- a/docs/source/configuration/traffic-shaping.mdx +++ b/docs/source/configuration/traffic-shaping.mdx @@ -43,6 +43,7 @@ traffic_shaping: min_per_sec: 10 # minimal number of retries per second (`min_per_sec`, default is 10 retries per second) ttl: 10s # for each successful request, we register a token, that expires according to this option (default: 10s) retry_percent: 0.2 # defines the proportion of available retries to the current number of tokens + retryable_mutations: false # allows retries on mutations. This should only be enabled if mutations are idempotent ``` From 9f0460ea1c40ffa3ffd73ef57a7e86e736d76659 Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Fri, 25 Nov 2022 15:21:33 +0100 Subject: [PATCH 2/3] changelog --- NEXT_CHANGELOG.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index c86d9ac0e8..70d241f7f5 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -196,6 +196,8 @@ bootstrap the system or for low traffic deployments - for each successful request, we add a "token" to the bucket, those tokens expire after `ttl` (default: 10 seconds) - the number of available additional retries is a part of the number of tokens, defined by `retry_percent` (default is 0.2) +Request retries are disabled by default on mutations. + This is activated in the `traffic_shaping` plugin, either globally or per subgraph: ```yaml @@ -205,13 +207,14 @@ traffic_shaping: min_per_sec: 10 ttl: 10s retry_percent: 0.2 + retryable_mutations: false subgraphs: accounts: experimental_retry: min_per_sec: 20 ``` -By [@Geal](https://github.com/Geal) in https://github.com/apollographql/router/pull/2006 +By [@Geal](https://github.com/Geal) in https://github.com/apollographql/router/pull/2006 and https://github.com/apollographql/router/pull/2160 ## 🐛 Fixes From c47aafdaf7b950ff3a54376aa00b8bd97445f214 Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Mon, 28 Nov 2022 13:28:26 +0100 Subject: [PATCH 3/3] retryable_mutations -> retry_mutations --- NEXT_CHANGELOG.md | 2 +- ...outer__configuration__tests__schema_generation.snap | 10 ++++++++++ apollo-router/src/plugins/traffic_shaping/mod.rs | 6 +++--- apollo-router/src/plugins/traffic_shaping/retry.rs | 8 ++++---- docs/source/configuration/traffic-shaping.mdx | 2 +- 5 files changed, 19 insertions(+), 9 deletions(-) diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index 70d241f7f5..8ebac04231 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -207,7 +207,7 @@ traffic_shaping: min_per_sec: 10 ttl: 10s retry_percent: 0.2 - retryable_mutations: false + retry_mutations: false subgraphs: accounts: experimental_retry: diff --git a/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap b/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap index 2c8fd06bca..9bb02fa171 100644 --- a/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap +++ b/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap @@ -2224,6 +2224,11 @@ expression: "&schema" "minimum": 0.0, "nullable": true }, + "retry_mutations": { + "description": "allows request retries on mutations. This should only be activated if mutations are idempotent. Disabled by default", + "type": "boolean", + "nullable": true + }, "retry_percent": { "description": "percentage of calls to deposit that can be retried. This is in addition to any retries allowed for via min_per_sec. Must be between 0 and 1000, default value is 0.2", "type": "number", @@ -2358,6 +2363,11 @@ expression: "&schema" "minimum": 0.0, "nullable": true }, + "retry_mutations": { + "description": "allows request retries on mutations. This should only be activated if mutations are idempotent. Disabled by default", + "type": "boolean", + "nullable": true + }, "retry_percent": { "description": "percentage of calls to deposit that can be retried. This is in addition to any retries allowed for via min_per_sec. Must be between 0 and 1000, default value is 0.2", "type": "number", diff --git a/apollo-router/src/plugins/traffic_shaping/mod.rs b/apollo-router/src/plugins/traffic_shaping/mod.rs index e87efa3410..bd0d277355 100644 --- a/apollo-router/src/plugins/traffic_shaping/mod.rs +++ b/apollo-router/src/plugins/traffic_shaping/mod.rs @@ -112,7 +112,7 @@ struct RetryConfig { retry_percent: Option, /// allows request retries on mutations. This should only be activated if mutations /// are idempotent. Disabled by default - retryable_mutations: Option, + retry_mutations: Option, } impl Merge for RetryConfig { @@ -123,7 +123,7 @@ impl Merge for RetryConfig { ttl: self.ttl.or(fallback.ttl), min_per_sec: self.min_per_sec.or(fallback.min_per_sec), retry_percent: self.retry_percent.or(fallback.retry_percent), - retryable_mutations: self.retryable_mutations.or(fallback.retryable_mutations), + retry_mutations: self.retry_mutations.or(fallback.retry_mutations), }, } } @@ -326,7 +326,7 @@ impl TrafficShaping { config.ttl, config.min_per_sec, config.retry_percent, - config.retryable_mutations, + config.retry_mutations, ); tower::retry::RetryLayer::new(retry_policy) }); diff --git a/apollo-router/src/plugins/traffic_shaping/retry.rs b/apollo-router/src/plugins/traffic_shaping/retry.rs index c212a2b06e..7a130502fb 100644 --- a/apollo-router/src/plugins/traffic_shaping/retry.rs +++ b/apollo-router/src/plugins/traffic_shaping/retry.rs @@ -11,7 +11,7 @@ use crate::services::subgraph; #[derive(Clone, Default)] pub(crate) struct RetryPolicy { budget: Arc, - retryable_mutations: bool, + retry_mutations: bool, } impl RetryPolicy { @@ -19,7 +19,7 @@ impl RetryPolicy { duration: Option, min_per_sec: Option, retry_percent: Option, - retryable_mutations: Option, + retry_mutations: Option, ) -> Self { Self { budget: Arc::new(Budget::new( @@ -27,7 +27,7 @@ impl RetryPolicy { min_per_sec.unwrap_or(10), retry_percent.unwrap_or(0.2), )), - retryable_mutations: retryable_mutations.unwrap_or(false), + retry_mutations: retry_mutations.unwrap_or(false), } } } @@ -44,7 +44,7 @@ impl Policy for RetryPolicy { None } Err(_e) => { - if req.operation_kind == OperationKind::Mutation && !self.retryable_mutations { + if req.operation_kind == OperationKind::Mutation && !self.retry_mutations { return None; } diff --git a/docs/source/configuration/traffic-shaping.mdx b/docs/source/configuration/traffic-shaping.mdx index 001790e7f3..7f8009d1f0 100644 --- a/docs/source/configuration/traffic-shaping.mdx +++ b/docs/source/configuration/traffic-shaping.mdx @@ -43,7 +43,7 @@ traffic_shaping: min_per_sec: 10 # minimal number of retries per second (`min_per_sec`, default is 10 retries per second) ttl: 10s # for each successful request, we register a token, that expires according to this option (default: 10s) retry_percent: 0.2 # defines the proportion of available retries to the current number of tokens - retryable_mutations: false # allows retries on mutations. This should only be enabled if mutations are idempotent + retry_mutations: false # allows retries on mutations. This should only be enabled if mutations are idempotent ```