Fix some graphman bugs and improve usability #3416
Conversation
node/src/manager/deployment.rs
Outdated
| let mut locators: Vec<DeploymentLocator> = HashSet::<DeploymentLocator>::from_iter( | ||
| Deployment::lookup(pool, name)? | ||
| .into_iter() | ||
| .map(|deployment| deployment.locator()), | ||
| ) | ||
| .into_iter() | ||
| .collect(); | ||
| let deployment_locator = match locators.len() { | ||
| 0 => anyhow::bail!("Found no deployment for `{}`", name), | ||
| 1 => locators.pop().unwrap(), | ||
| n => anyhow::bail!("Found {} deployments for `{}`", n, name), | ||
| }; |
There was a problem hiding this comment.
Interesting!
I didn't realize a deployment could have multiple names.
Makes total sense now.
There was a problem hiding this comment.
Yeah, that's what keeps things interesting. When people deploy an IPFS hash that was already running under a different name, we just put an entry in subgraph and subgraph_version to point to that deployment. So the subgraph name -> deployment mapping is many-to-one
node/src/manager/deployment.rs
Outdated
| let deployments: Vec<Deployment> = match self { | ||
| DeploymentSearch::Name { name } => { | ||
| let pattern = format!("%{}%", name); | ||
| query.filter(s::name.ilike(&pattern)).load(conn)? | ||
| } | ||
| DeploymentSearch::Hash { hash, shard } => { | ||
| let query = query.filter(ds::subgraph.eq(&hash)); | ||
| match shard { | ||
| Some(shard) => query.filter(ds::shard.eq(shard)).load(conn)?, | ||
| None => query.load(conn)?, | ||
| } | ||
| } | ||
| DeploymentSearch::Deployment { namespace } => { | ||
| query.filter(ds::name.eq(&namespace)).load(conn)? | ||
| } | ||
| }; |
| fn locate(store: &dyn SubgraphStore, hash: &str) -> Result<DeploymentLocator, anyhow::Error> { | ||
| let mut locators = store.locators(&hash)?; | ||
| match locators.len() { | ||
| 0 => bail!("could not find subgraph {hash} we just created"), | ||
| 1 => Ok(locators.pop().unwrap()), | ||
| n => bail!("there are {n} subgraphs with hash {hash}"), | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This "get a single item from a collection" pattern happens so often that I created a convenience function for it here.
In the future, I'll try to make that even more generic/flexible so we can reuse it.
There was a problem hiding this comment.
The reason this is special and not used with DeploymentSearch is that that wants the primary pool, and we only have a SubgraphStore here. The run code could probably be changed to also access the primary pool. But it's a smaller issue (and run needs a lot more TLC anyway)
| None => { | ||
| println!("assigning {locator} to {node}"); | ||
| conn.assign_subgraph(&site, &node)?; | ||
| } |
|
While reviewing this, I noticed that the graph-node/graph/src/components/store/mod.rs Lines 762 to 767 in 9da5493 Do you think we could prevent this by changing its inner field visibility and implementing a getter? -pub struct DeploymentId(pub i32);
+pub struct DeploymentId(pub(crate) i32);
...
+pub fn id(&self) -> i32 { self.0 }Please let me know if this would be useful, and I'll come up with a follow-up PR :) |
I don't have strong opinions either way, but a getter feels cleaner than than just |
When looking for a deployment, like `sgdNNNN`, the code would complain about it being ambiguous if that deployment was mapped to multiple names, even though the name denotes a unique deployment.
The current way of looking up deployments is pretty adhoc. The `DeploymentSearch` struct will make it possible to have the same mechanism across all commands.
Also, get rid of the weird parsing of the table name in `stats account-like`
- the deployment is now specified with DeploymentSearch - reassign now works for both already assigned and unassigned subgraphs
The
graphman indexcommands wouldn't work if the same deployment is used by multiple names.This PR also overhauls the logic for how deployments are looked up to make the lookup uniform across various commands