Conversation
leoyvens
left a comment
leoyvens
left a comment
There was a problem hiding this comment.
For all I can tell, we can refactor BlockStreamEvent::Revert to always require a parent block ptr, rather than taking an Option.
|
Better than that, it was actually already fetched once in The code in instance manager is not required anymore actually. |
|
Done. |
leoyvens
left a comment
leoyvens
left a comment
There was a problem hiding this comment.
All DB operations to process a revert should be done in the same DB transaction, so we can rely on ACID guarantees. So the update_block_cursor operation should be done in the same transaction as the other operations.
|
@leoyvens You are right, makes total sense. I've updated the PR. While doing the operations, while updating where
|
|
For the purpose of this PR, we need to at least understand if it's safe to call |
… the databse - On revert operation, `parent_ptr` everywhere since it avoids a useless RPC (or in cache database) call. - The firehose update cursor on remove is directly in `revert_block_operations` to be in a transaction alongside the update of the subgraph ptr.
maoueh
force-pushed
the
maoueh/fix/firehose-save-cursor-on-revert
branch
from
845432d to
43a3166
Compare
|
As discussed last week, for If an upcoming PR, I'm going to make it possible for firehose to start from a I think the PR is good to merge in it's current shape and next improvement will come in a subsequent PR. |
| conn: &PgConnection, | ||
| site: Arc<Site>, | ||
| block_ptr_to: BlockPtr, | ||
| firehose_cursor: Option<&str>, |
There was a problem hiding this comment.
Should this still be an Option, given this is now using the empty string?
There was a problem hiding this comment.
Well, I used "" at some place to "clear" the cursor, because right now None means do nothing (which is the case when using Ethereum RPC).
So I'm of the opinion that we should keep it like this.
2 participants
This PR ensures that the Firehose cursor is saved properly even on revert operations.