Skip to content

Commit

Permalink
fix(dev): intermediate composition states have noisy errors (#1956)
Browse files Browse the repository at this point in the history
### notes 
- the way we're composing subgraphs with `dev --supergraph-config` means
that there are intermediate states of composition, and sometimes those
intermediate states fail composition because they require other
subgraphs to be present
  - lots of errors show up, but they're only ephemeral
- fixes those errors by waiting until we've at least as many subgraphs
loaded in as are represented by the supergraph.yaml, running composition
only when we've at least those many
  - #1919

### before:
<img width="730" alt="Screenshot 2024-07-03 at 4 51 44 PM"
src="https://github.com/apollographql/rover/assets/26738844/8429a155-8c23-49fe-b7e4-da16fe33d71c">


### after

<img width="708" alt="Screenshot 2024-07-03 at 4 50 15 PM"
src="https://github.com/apollographql/rover/assets/26738844/084b5bf7-9182-42c9-944e-409c1582ba64">

### testing

I added three new subgraphs to the supergraph example; there's a branch
with the same code as this one
(`aaron/fix-intermediate-composition-errors-testing`), but with those
new subgraphs added. Comment out the subgraph length check to see the
failure; from within the examples directory, run `cargo r -- dev
--supergraph-config ./supergraph.yaml`. You might have to run it a
couple times to see the failure because of the thread pool
  • Loading branch information
aaronArinder authored Jul 10, 2024
1 parent e2bd757 commit 880b307
Showing 1 changed file with 24 additions and 4 deletions.
28 changes: 24 additions & 4 deletions src/command/dev/protocol/leader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ pub struct LeaderSession {
follower_channel: FollowerChannel,
leader_channel: LeaderChannel,
federation_version: FederationVersion,
supergraph_config: Option<SupergraphConfig>,
}

impl LeaderSession {
Expand Down Expand Up @@ -141,6 +142,7 @@ impl LeaderSession {
follower_channel,
leader_channel,
federation_version,
supergraph_config: supergraph_config.clone(),
}))
}

Expand Down Expand Up @@ -267,6 +269,23 @@ impl LeaderSession {

if let Vacant(e) = self.subgraphs.entry((name.to_string(), url.clone())) {
e.insert(sdl.to_string());

// Followers add subgraphs, but sometimes those subgraphs depend on each other
// (e.g., through extending a type in another subgraph). When that happens,
// composition fails until _all_ subgraphs are loaded in. This acknowledges the
// follower's message when we haven't loaded in all the subgraphs, deferring
// composition until we have at least the number of subgraphs represented in the
// supergraph.yaml file
//
// This applies only when the supergraph.yaml file is present. Without it, we will
// try composition each time we add a subgraph
if let Some(supergraph_config) = self.supergraph_config.clone() {
let subgraphs_from_config = supergraph_config.into_iter();
if self.subgraphs.len() < subgraphs_from_config.len() {
return LeaderMessageKind::MessageReceived;
}
}

let composition_result = self.compose();
if let Err(composition_err) = composition_result {
LeaderMessageKind::error(composition_err)
Expand Down Expand Up @@ -335,7 +354,7 @@ impl LeaderSession {
/// Reruns composition, which triggers the router to reload.
fn compose(&mut self) -> CompositionResult {
self.compose_runner
.run(&mut self.supergraph_config())
.run(&mut self.supergraph_config_internal_representation())
.and_then(|maybe_new_schema| {
if maybe_new_schema.is_some() {
if let Err(err) = self.router_runner.spawn() {
Expand Down Expand Up @@ -374,9 +393,10 @@ impl LeaderSession {
socket_write(&message, stream)
}

/// Gets the supergraph configuration from the internal state.
/// Calling `.to_string()` on a [`SupergraphConfig`] writes
fn supergraph_config(&self) -> SupergraphConfig {
/// Gets the supergraph configuration from the internal state. This can different from the
/// supergraph.yaml file as it represents intermediate states of composition while adding
/// subgraphs to the internal representation of that file
fn supergraph_config_internal_representation(&self) -> SupergraphConfig {
let mut supergraph_config: SupergraphConfig = self
.subgraphs
.iter()
Expand Down

0 comments on commit 880b307

Please sign in to comment.