fix: Allow grafts to add data sources #3989
Conversation
| } | ||
| } | ||
|
|
||
| pub fn template_idx_and_name(&self) -> Result<Vec<(i32, String)>, Error> { |
There was a problem hiding this comment.
I understand we're using i32 to simplify comparing these values with query results, but it would be nice to postpone the conversion and create a type alias for this tuple.
There was a problem hiding this comment.
Could be but I'll be lazy and leave it for now
| .load::<Tuple>(conn)?; | ||
|
|
||
| let mut count = 0; | ||
| for (block_range, src_manifest_idx, param, context, causality_region) in src_tuples { |
There was a problem hiding this comment.
I suspect these N+1 queries may take longer than we'd like, but I don't have a better idea and N is most likely small for most subgraphs.
lutter
left a comment
lutter
left a comment
There was a problem hiding this comment.
The general approach looks good! Just some smaller detail comments on how we go about storing the raw yaml.
| ($($err:tt)*) => { | ||
| return Err(anyhow::anyhow!($($err)*).into()); | ||
| }; | ||
| } |
There was a problem hiding this comment.
Why don't we just use the existing anyhow::bail! macro? If this is truly needed, maybe add a comment why.
There was a problem hiding this comment.
It doesn't call .into(), yhea needs a comment.
| ) -> Result<(), StoreError> { | ||
| use subgraph_manifest as sm; | ||
|
|
||
| update(sm::table.filter(sm::id.eq(site.id))) |
There was a problem hiding this comment.
It would be better to also include .filter(sm::raw_yaml.ne(raw_yaml)) to avoid unnecessary updates in that table. Otherwise, we'd rewrite the entire table on each node start.
There was a problem hiding this comment.
Good catch, I've added a .filter(sm::raw_yaml.is_null()) clause.
There was a problem hiding this comment.
Sounds good! The sideeffect is that we'll never update the raw_yaml once it's set, but that shouldn't be an issue
leoyvens
force-pushed
the
leo/grafting-add-data-source
branch
2 times, most recently
from
2768f86 to
21e9548
Compare
leoyvens
force-pushed
the
leo/grafting-add-data-source
branch
from
21e9548 to
cf6d023
Compare
|
Thanks for the reviews, @lutter I've addressed your comments. |
Resolves #3960. This is done by adding a
raw_yamlcolumn to thesubgraph_manifesttable, and using it to map the manifest idx from the source to the destination deployment. For existing deployments this column is migrated in the instance manager on subgraph startup. This also changes a bit the order in which things are done in the instance manager, now the subgraph is first fetched from IPFS before being started in the store.The
data-source-revertintegration test was modified to also test this.