Update nonFatalErrors in subgraphs.subgraph_deployment table #4615
Conversation
incrypto32
force-pushed
the
incrypto32/fix-non-fatal-errors-not-populating
branch
from
458997c
to
3760a00
Compare
incrypto32
force-pushed
the
incrypto32/fix-non-fatal-errors-not-populating
branch
3 times, most recently
from
aac2ad3
to
7154d87
Compare
There was a problem hiding this comment.
I left a few stylistic comments that would be good to address before merging, but other than that this looks good.
It would also be good to have a test that checks that the non-fatal errors on a deployment get set now when that should happen (it's enough if this is just a test within the store) so we don't accidentally regress.
One thing I am not clear on is whether fail_subgraph still needs to also store the error - it seems like that is just additional work now?
core/src/subgraph/runner.rs
Outdated
| @@ -468,6 +468,7 @@ where | |||
| } = block_state; | |||
|
|
|||
| let first_error = deterministic_errors.first().cloned(); | |||
| let deterministic_errors = Arc::new(deterministic_errors); | |||
There was a problem hiding this comment.
You shouldn't put this into an Arc; that way, you don't have to call clone().to_vec() below and can just pass deterministic_errors into transact_block_operations
There was a problem hiding this comment.
got it, thanks for pointing it out, did so since previously non_fatal_errors were being written in the same function and deterministic_errors get moved, now we do that in transact_block_operations so its no longer required to be Arc
| @@ -1118,6 +1118,16 @@ impl DeploymentStore { | |||
| )?; | |||
| } | |||
|
|
|||
| if !batch.deterministic_errors.is_empty() && batch.is_non_fatal_errors_active { | |||
There was a problem hiding this comment.
It would be better to pull this if into the previous if statement so that you only have to say if batch.is_non_fatal_errors_active { .. }
store/postgres/src/deployment.rs
Outdated
| conn: &PgConnection, | ||
| deployment_id: &DeploymentHash, | ||
| health: SubgraphHealth, | ||
| non_fatal_errors: Option<Vec<SubgraphError>>, |
There was a problem hiding this comment.
If you make this an Option<&[SubgraphError]> the caller doesn't need to clone - it's not a big deal in this case since errors happen rarely, but on hotter code paths it would make a difference
@lutter in this case As you said since it works and ignores the duplicate insert silenty, we should not touch |
incrypto32
force-pushed
the
incrypto32/fix-non-fatal-errors-not-populating
branch
2 times, most recently
from
3892a76
to
6838968
Compare
incrypto32
force-pushed
the
incrypto32/fix-non-fatal-errors-not-populating
branch
from
6838968
to
92451cc
Compare
|
@lutter i have added tests, transact some errors and then check that status to check if nonFatalErrors array has something in there. Also squashed the commits into two. |
| let count = || -> usize { | ||
| let store = store.subgraph_store(); | ||
| let count = store.error_count(&subgraph_id).unwrap(); | ||
| println!("count: {}", count); |
There was a problem hiding this comment.
Looks like a leftover from debugging.
|
|
||
| let info = subgraph_store.status_for_id(deployment.id); | ||
|
|
||
| assert!(info.non_fatal_errors.len() == 1); |
There was a problem hiding this comment.
It's generally better to use assert_eq!(1, info.non_fatal_errors.len()) for equality comparisons since that will print both the expected and actual values when the assertion fails. It's a small quality-of-life improvement when tests fail, not that big a deal, but would be good to change before merging.
Solves #3350