-
Notifications
You must be signed in to change notification settings - Fork 20
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
Properly implement subgraph client fallback from local deployment to remote URL #81
Conversation
Makes sense! Here's how I understood it:
If I understood it correctly, then shouldn't that be extended further, and the SubgraphClient only point to a user-provided |
PS: and the actual path to the correct subgraph endpoint in indexer-agent would be hardcoded, ie. something like: let indexer_agent_subgraphs_endpoint = "http://localhost:8080/subgraphs/" // (from config)
let network_subgraph = SubgraphClient::from_something(indexer_agent_subgraphs_endpoint, "escrow-subgraph"); |
common/src/subgraph_client.rs
Outdated
|
||
let subgraph_url = Url::parse(subgraph_url) | ||
.map_err(|e| anyhow!("Could not parse subgraph url `{}`: {}", subgraph_url, e))?; | ||
pub fn new(name: &str, query_url: &str) -> Result<Self, anyhow::Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need name
? It seems to be only used for error messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, no, let's not include it.
I re-checked what the existing indexer-service does and it's a bit more complicated. It essentially polls the status of the local deployment and uses that if synced, otherwise it uses the remote subgraph. I think we need at least a local and remote URL for the network subgraph (same with escrow ideally). I'll rework this PR to match the existing behavior. |
Pull Request Test Coverage Report for Build 6628418855
💛 - Coveralls |
This is how the existing indexer-common does it - it uses the local network subgraph if it's synced and healthy, otherwise falls back to a remote subgraph URL.
ac6dcf5
to
460c6bd
Compare
Oh, ok then! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just one comment in the code that may be obsolete.
Added a bunch more tests for the fallback behavior. This should be good to go now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may deviate from the existing indexer-service, but that one has grown over time. Really, even if you index the network subgraph deployment (or escrow subgraph) locally, you just need a query URL in the indexer service. The optional deployment logic is handled by indexer-agent.This now implements the correct logic to monitor the local subgraph deployment status and, if it's not healthy and synced, falls back to the remote URL provided. This mimics the behavior of the existing indexer service: https://github.com/graphprotocol/indexer/blob/main/packages/indexer-common/src/network-subgraph.ts#L143-L167.