Enable Retries for Transient Errors Connecting to Graphs/Subgraphs #1936

jonathanrainer · 2024-06-20T11:17:23Z

This PR fixes two separate issues identified with the way Rover handles retries for communicating with graphs/subgraphs when using rover dev.

Under normal operations subgraphs will get disconnected from the federation upon the first receipt of an error, this is slightly too eager and now is configurable with the new subgraph-retries option.
1. This had the side effect that composition was running a huge amount of the time causing CPU spikes.
Under normal operations we did not have retries enabled for transient network issues, this is now fixed
1. If this persists there is another solution which is to turn off connection pooling completely, however fixing these transient issues may render this unnecessary.

nmoutschen · 2024-06-20T11:25:28Z

src/command/dev/watcher.rs

                            self.message_sender.update_subgraph(&subgraph_definition)?;
                        }
                    }
                    None => {
                        self.message_sender.add_subgraph(&subgraph_definition)?;
                    }
                }
+                self.subgraph_retry_countdown = self.subgraph_retries;


Nit: Should we document the logic on how retry_countdown works?

For my understanding:

It is normally at 0

On success, we re-initialise it to subgraph_retries

On every attempt, we decrement until it reaches 0

This means that if the first attempt is a failure, we stay at 0, thus go into the branch at line 225

nmoutschen · 2024-06-20T11:26:34Z

src/options/subgraph.rs

+    #[arg(long = "subgraph-retry", short = 'r', default_value = "1")]
+    #[serde(skip_serializing)]
+    pub subgraph_retry: u64,


Nit: should this be pluralised? There is a bit of naming inconsistency in this PR between retry and retries

Suggested change

#[arg(long = "subgraph-retry", short = 'r', default_value = "1")]

#[serde(skip_serializing)]

pub subgraph_retry: u64,

#[arg(long = "subgraph-retries", short = 'r', default_value = "1")]

#[serde(skip_serializing)]

pub subgraph_retries: u64,

nmoutschen · 2024-06-20T11:27:55Z

src/utils/client.rs

@@ -64,6 +64,7 @@ impl ClientBuilder {
            .danger_accept_invalid_hostnames(self.accept_invalid_hostnames)
            .timeout(self.timeout)
            .user_agent(format!("{}/{}", PKG_NAME, PKG_VERSION))
+            .pool_max_idle_per_host(0)


Question: should we keep this change in this PR?

There might be unknown side-effects with this change, so I'd prefer to remove it, but re-add it at a later time if we see this is needed.

jackonawalk · 2024-06-20T14:52:05Z

src/command/dev/watcher.rs

+                //
+                if self.subgraph_retry_countdown > 0 {
+                    self.subgraph_retry_countdown -= 1;
+                    eprintln!("{} error detected communicating with subgraph '{}', schema changes will not be reflected.\nWill retry but subgraph logs should be inspected", Emoji::Warn, &self.subgraph_key.0);


to confirm, schema changes won't be saved because introspection couldn't run or because rover couldn't confirm the schema with the subgraph?

So more likely the former, essentially this happens when the introspection query fails for whatever reason (4xx, 5xx and so on), so we don't get back a new schema to check against.

jackonawalk · 2024-06-20T14:52:23Z

src/command/dev/watcher.rs

+                    Some(e.to_string())
+                } else {
+                    eprintln!(
+                        "{} retries exhausted for subgraph {}",


I think this is fine. how many retries does the user get?

It's configurable with the --subgraph-retries parameter but I can easily add that into the log message so people know it's being respected?

This way we avoid subgraphs potentially being removed for spurious reasons. In addition enable it for 'graphs` as well so that we don't end up only partially fixing this

This should not be so eager to do so, and is now in fact configurable.

@jonathanrainer

# [0.25.0] - 2024-07-22 ## 🚀 Features - **Enable Retries For Transient Errors Connecting To Graphs/Subgraphs - @jonathanrainer PR #1936** This turns on retries at the HTTP level for connections to graphs/subgraphs to minimize connection resets and cancellations. Also, a new --subgraph-retries flag for rover dev lets you set the number of retries allowed when trying to re-establish a connection. - **Add `--graph-ref` flag to `rover dev` - @dotdat PR #1984** Introduces subgraph mirroring to rover dev. Subgraph mirroring inherits the subgraph routing URLs and schemas from an existing Studio graphref. This makes it easy to spin up a locally running supergraph without maintaining a supergraph config. [See here](https://www.apollographql.com/docs/rover/commands/dev#starting-a-session-from-a-graphos-studio-variant) for more information. ## 🐛 Fixes - **Fixes issues related to passing filenames to `--output` - @jonathanrainer PR #1996** An issue was raised whereby previous versions of Rover supported passing filenames to the `--output` flag but this was broken in v0.24.0. This has now been fixed and the previous functionality restored. ## 🛠 Maintenance - **Expand Smoke Tests To Run On All Supported Platforms - @jonathanrainer PR #1980** - **Fix cron expression, so it runs only once per day - @jonathanrainer PR #1986** - **Ensure we always use the correct version of Federation when testing - @jonathanrainer PR #1987** - **Add manual Smoke test invocation and pin Windows to `npm@9` for testing - @jonathanrainer PR #1989** - **Update apollographql/router to v1.51.0 - @jonathanrainer PR #1988** - **Update node.js packages - @jonathanrainer PR #1979** Includes `@eslint/compat` to v1.1.1, `eslint` to v9.7.0, `node.js` to v20.15.1, `npm` to v10.8.2 and `prettier` to v3.3.3 - **Make sure x86 Mac Tests use 'latest' supergraph plugin version - @jonathanrainer PR #1990** - **Make sure homebrew runs `brew update` when we use it - @jonathanrainer PR #1993** ## 📚 Documentation - **Adds `graph-ref` flag to dev subcommand docs - @jackonawalk PR #1945** - **Update schema proposals capabilities docs - @Meschreiber PR #1949**

nmoutschen reviewed Jun 20, 2024

View reviewed changes

jonathanrainer requested a review from nmoutschen June 20, 2024 11:39

nmoutschen approved these changes Jun 20, 2024

View reviewed changes

jonathanrainer force-pushed the jr/bug/ROVER-44 branch 2 times, most recently from 0f9b427 to e740f7f Compare June 20, 2024 11:54

jonathanrainer marked this pull request as ready for review June 20, 2024 12:04

jonathanrainer requested a review from a team as a code owner June 20, 2024 12:04

jackonawalk reviewed Jun 20, 2024

View reviewed changes

jonathanrainer mentioned this pull request Jun 26, 2024

Intermittent introspection query errors in rover dev #1868

Open

jonathanrainer force-pushed the jr/bug/ROVER-44 branch 3 times, most recently from d7306c4 to 38bf054 Compare July 1, 2024 12:50

jonathanrainer added this to the vNext milestone Jul 10, 2024

jonathanrainer added the feature 🎉 new commands, flags, functionality, and improved error messages label Jul 13, 2024

jonathanrainer force-pushed the jr/bug/ROVER-44 branch 2 times, most recently from 5673f1a to 3bd20d1 Compare July 15, 2024 13:23

jonathanrainer added 4 commits July 15, 2024 15:03

ROVER-44 Set should_retry to true

03c3233

This way we avoid subgraphs potentially being removed for spurious reasons. In addition enable it for 'graphs` as well so that we don't end up only partially fixing this

ROVER-44 Update logic for removing subgraphs

053a3a2

This should not be so eager to do so, and is now in fact configurable.

ROVER-44 Responding to PR comments

4e8567e

Fix up log messages to expose flag

75003aa

jonathanrainer force-pushed the jr/bug/ROVER-44 branch from 3bd20d1 to 75003aa Compare July 15, 2024 14:03

jonathanrainer merged commit da07a74 into main Jul 15, 2024
18 checks passed

jonathanrainer deleted the jr/bug/ROVER-44 branch July 15, 2024 14:13

jonathanrainer mentioned this pull request Jul 19, 2024

release: v0.25.0 #1997

Merged

WontonSam mentioned this pull request Aug 14, 2024

[Snyk] Upgrade @apollo/rover from 0.14.2 to 0.25.0 WontonSam/apollo-federation-subgraph-compatibility#416

Open

WontonSam mentioned this pull request Sep 15, 2024

[Snyk] Upgrade: , , , , , execa, make-fetch-happen WontonSam/apollo-federation-subgraph-compatibility#512

Open

WontonSam mentioned this pull request Sep 27, 2024

[Snyk] Upgrade @apollo/rover from 0.14.2 to 0.26.1 WontonSam/apollo-federation-subgraph-compatibility#554

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Retries for Transient Errors Connecting to Graphs/Subgraphs #1936

Enable Retries for Transient Errors Connecting to Graphs/Subgraphs #1936

jonathanrainer commented Jun 20, 2024 •

edited

Loading

nmoutschen Jun 20, 2024

nmoutschen Jun 20, 2024

nmoutschen Jun 20, 2024

jackonawalk Jun 20, 2024

jonathanrainer Jun 21, 2024

jackonawalk Jun 20, 2024

jonathanrainer Jun 21, 2024

Enable Retries for Transient Errors Connecting to Graphs/Subgraphs #1936

Enable Retries for Transient Errors Connecting to Graphs/Subgraphs #1936

Conversation

jonathanrainer commented Jun 20, 2024 • edited Loading

nmoutschen Jun 20, 2024

Choose a reason for hiding this comment

nmoutschen Jun 20, 2024

Choose a reason for hiding this comment

nmoutschen Jun 20, 2024

Choose a reason for hiding this comment

jackonawalk Jun 20, 2024

Choose a reason for hiding this comment

jonathanrainer Jun 21, 2024

Choose a reason for hiding this comment

jackonawalk Jun 20, 2024

Choose a reason for hiding this comment

jonathanrainer Jun 21, 2024

Choose a reason for hiding this comment

jonathanrainer commented Jun 20, 2024 •

edited

Loading