Allow redeployment of grafted subgraph even when graft_base is not available #4695

incrypto32 · 2023-06-14T06:22:36Z

store/postgres/src/subgraph_store.rs

…base is not available

leoyvens · 2023-06-15T13:56:44Z

store/postgres/src/deployment_store.rs

@@ -1441,6 +1441,11 @@ impl DeploymentStore {
        .await
    }

+    pub(crate) fn exists(&self, id: Arc<Site>) -> Result<bool, StoreError> {
+        let conn = self.get_conn()?;


Lets use with_conn here to make this async.

@leoyvens i tried that initially wouldn't that mean i would have to make all the callers also async function to await on it?

I was unsure if i should do that, let me know if its ok.

Are there any non-async callers? If there are then we'd need to keep this one and add an async version.

fn create_deployment_internal is not async but its callers can be made async ( cause callers all the way is async ). Should i make them all async in that case?

I see now that this is a bigger problem since fn create_deployment_internal is doing many sync DB operations, but is called from an async context. Ok we can leave this exists as sync for now, and later find a solution for the whole function.

leoyvens · 2023-06-15T15:53:17Z

store/postgres/src/subgraph_store.rs

+            layout.site.schema_version
+        } else {
+            DeploymentSchemaVersion::LATEST
+        };


I think we could avoid the need for a schema_version variable here by having fn allocate_site take a graft_base parameter instead of schema_version, and do the logic there.

fn allocate_site already checks for existence, and if it needs to search for the graft base I believe it could use fn find_active_site.

got it, will make that change

leoyvens · 2023-06-15T17:00:31Z

core/src/subgraph/registrar.rs

        raw,
        resolver,
        logger,
        ENV_VARS.max_spec_version.clone(),
    )
    .map_err(SubgraphRegistrarError::ResolveError)
    .await?;
-
+    let exists = store.is_deployed(&deployment)?;


There is an fn graft_pending which we perhaps should additionally check, because it checks if the subgraph has actually finished the graft process. It would be nice if we were able to use that here.

incrypto32 · 2023-06-20T12:54:10Z

Integration tests, failing.. checking it out.

leoyvens

Looks great! Thanks for bearing with the reviews

leoyvens · 2023-06-20T15:22:10Z

core/src/subgraph/registrar.rs

+        Ok(graft_pending) => graft_pending,
+        Err(StoreError::DeploymentNotFound(_)) => true,
+        Err(e) => return Err(SubgraphRegistrarError::StoreError(e)),
+    };


Would be great to have comments here.

leoyvens · 2023-06-20T15:23:06Z

store/postgres/src/primary.rs

@@ -1247,18 +1247,33 @@ impl<'a> Connection<'a> {
    }

    /// Create a site for a brand new deployment.
+    /// If it already exists, return the existing site.
+    /// and a boolean indicating whether a new site was created.
+    /// false means the site already existed.


Nice comment, just need to check ponctuation and capitalization.

ah thanks for catching that. I'll fix that

leoyvens · 2023-06-20T16:28:39Z

core/src/subgraph/registrar.rs

+    // Determine if the subgraph should be validated.
+    // Validate the subgraph if there is a pending graft, indicating the presence of a graft base.
+    // If the subgraph is new (DeploymentNotFound), it should also be validated.
+    // If the subgraph already exists and there is no pending graft, validation is not required.


The wording here is not very precise, this boolean is not deciding if the whole subgraph should be validated, but if the graft base should be validated.

// Determine if the graft_base should be validated.
// Validate the graft_base if there is a pending graft, ensuring its presence.
// If the subgraph is new (indicated by DeploymentNotFound), the graft_base should be validated.
// If the subgraph already exists and there is no pending graft, graft_base validation is not required.

@leoyvens how does this look?

incrypto32 force-pushed the incrypto32/graft-redeploy-fix branch 2 times, most recently from fd791dd to 8d2ca4d Compare June 14, 2023 06:35

incrypto32 marked this pull request as ready for review June 14, 2023 06:40

incrypto32 changed the title ~~redeployment of grafted subgraph even when graft_base is not available~~ Allow redeployment of grafted subgraph even when graft_base is not available Jun 14, 2023

incrypto32 requested review from tilacog and leoyvens June 14, 2023 06:48

tilacog reviewed Jun 14, 2023

View reviewed changes

store/postgres/src/subgraph_store.rs Outdated Show resolved Hide resolved

incrypto32 force-pushed the incrypto32/graft-redeploy-fix branch from 8d2ca4d to 6ea0edf Compare June 14, 2023 15:43

core, store : Allow redeployment of grafted subgraph even when graft_…

a54409d

…base is not available

incrypto32 requested a review from tilacog June 14, 2023 15:44

incrypto32 force-pushed the incrypto32/graft-redeploy-fix branch from 6ea0edf to a54409d Compare June 14, 2023 15:44

incrypto32 self-assigned this Jun 15, 2023

leoyvens reviewed Jun 15, 2023

View reviewed changes

store,core,graph : move schema_version calculation to allocate_site

2d372c9

incrypto32 requested a review from leoyvens June 19, 2023 15:26

incrypto32 force-pushed the incrypto32/graft-redeploy-fix branch from dde11bc to 8ccd608 Compare June 20, 2023 11:11

store : move subgraph exist logic into allocate_site

5062fe8

incrypto32 force-pushed the incrypto32/graft-redeploy-fix branch from 8ccd608 to 5062fe8 Compare June 20, 2023 14:27

leoyvens approved these changes Jun 20, 2023

View reviewed changes

incrypto32 force-pushed the incrypto32/graft-redeploy-fix branch from f999321 to c73fc91 Compare June 20, 2023 16:18

leoyvens reviewed Jun 20, 2023

View reviewed changes

core: use existence check in graft_pending

f275665

incrypto32 force-pushed the incrypto32/graft-redeploy-fix branch from c73fc91 to f275665 Compare June 21, 2023 05:23

leoyvens approved these changes Jun 21, 2023

View reviewed changes

incrypto32 merged commit 510797c into master Jun 21, 2023
6 checks passed

incrypto32 deleted the incrypto32/graft-redeploy-fix branch June 21, 2023 09:15

tilacog mentioned this pull request Jun 22, 2023

L2 support for Indexer Agent graphprotocol/indexer#668

Merged

6 tasks

tilacog mentioned this pull request Aug 11, 2023

Review if the GraphNode class needs an array of index node ids graphprotocol/indexer#714

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow redeployment of grafted subgraph even when graft_base is not available #4695

Allow redeployment of grafted subgraph even when graft_base is not available #4695

incrypto32 commented Jun 14, 2023

leoyvens Jun 15, 2023

incrypto32 Jun 15, 2023

incrypto32 Jun 15, 2023

leoyvens Jun 16, 2023

incrypto32 Jun 16, 2023

leoyvens Jun 16, 2023

leoyvens Jun 15, 2023

incrypto32 Jun 15, 2023

leoyvens Jun 15, 2023

incrypto32 commented Jun 20, 2023

leoyvens left a comment

leoyvens Jun 20, 2023

leoyvens Jun 20, 2023

incrypto32 Jun 20, 2023

leoyvens Jun 20, 2023

incrypto32 Jun 20, 2023

Allow redeployment of grafted subgraph even when graft_base is not available #4695

Allow redeployment of grafted subgraph even when graft_base is not available #4695

Conversation

incrypto32 commented Jun 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

incrypto32 commented Jun 20, 2023

leoyvens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment