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.

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

Further refactor adapter handling, BlockStream, BlockIngestor #4411

Merged
merged 1 commit into from
Mar 6, 2023

Conversation

mangas
Copy link
Contributor

@mangas mangas commented Mar 2, 2023

  1. BlockIngestor is now a trait and each Blockchain will built it’s own
  2. BlockStream is now also built via a single function on the blockchain trait instead of leaking the firehose/non firehose decision
  3. The adapter is now not sticky on the FirehoseBlockStream ie it will retry with different adapters
  4. The FirehoseBlockStream now has the provider and subgraph id on every message
  5. Make FirehoseBlockIngestor adapter not sticky

Closes:
#4415
#4416

@mangas mangas marked this pull request as ready for review March 2, 2023 15:11
@mangas mangas changed the title Update chains init code Further refactor adapter handling, BlockStream, BlockIngestor Mar 2, 2023
@mangas mangas requested review from neysofu and lutter March 2, 2023 16:59
chain/ethereum/src/chain.rs Outdated Show resolved Hide resolved
chain/ethereum/src/ingestor.rs Outdated Show resolved Hide resolved
graph/src/blockchain/mod.rs Show resolved Hide resolved
graph/src/blockchain/mod.rs Show resolved Hide resolved
node/src/main.rs Outdated Show resolved Hide resolved
node/src/opt.rs Show resolved Hide resolved
Copy link
Collaborator

@lutter lutter left a comment

Choose a reason for hiding this comment

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

My main concern with this PR is that it changes some connection errors during startup into panics. We've had situations where it was not possible to start the block ingestor for all chains successfully, and we should make sure that that continues to work even if some chain clients have lost their mind.

node/src/main.rs Outdated
.collect(),
)
}
fn chains_to_ingestor(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function seems to not be used, no idea why rustc doesn't spot that

node/src/main.rs Outdated Show resolved Hide resolved
chain/substreams/src/chain.rs Outdated Show resolved Hide resolved
chain/arweave/src/chain.rs Outdated Show resolved Hide resolved
chain/ethereum/src/chain.rs Outdated Show resolved Hide resolved
chain/ethereum/src/env.rs Show resolved Hide resolved
graph/src/blockchain/block_stream.rs Show resolved Hide resolved
graph/src/blockchain/firehose_block_stream.rs Outdated Show resolved Hide resolved
graph/src/blockchain/mock.rs Outdated Show resolved Hide resolved
node/src/main.rs Outdated
$acc.push(ingestor);
}
Err(err) => error!(&logger,
"Failed to create block ingestor for network {}",err),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better wording: "Failed to create block ingestor: {}", err

- Move BlockIngestor init done by Blockchain trait
- BlockIngestor is now a trait
- BlockStream init now done by Blockchain trait
- Adapter not sticky on BlockIngestor and BlockStream
@mangas mangas merged commit 5c22a5c into master Mar 6, 2023
@mangas mangas deleted the chains-refactor branch March 6, 2023 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants