Fix firstEventBlockNumber bug in metadata - #124
Conversation
| let firstEventBlockNumber = switch parsedQueueItems[0] { | ||
| | Some(item) if chainFetcher.firstEventBlockNumber->Option.isNone => item.blockNumber->Some | ||
| | _ => chainFetcher.firstEventBlockNumber | ||
| } | ||
|
|
There was a problem hiding this comment.
Previously this number was just being set on chain fetcher here. Either it existed or it didn't. Now that there can be multiple partitions even when the indexer starts up, the first batch to come back would not necessarily contain the first event.
| //of a rollback. | ||
| dynamicContracts: DynamicContractsMap.t, | ||
| isFetchingAtHead: bool, | ||
| firstEventBlockNumber: option<int>, |
There was a problem hiding this comment.
Solution is to add firstEventBlockNumber to each partition and evaluate the earliest across partitions.
| let getFirstEventBlockNumber = ({partitions}: t) => { | ||
| partitions->List.reduce(None, (accum, partition) => { | ||
| Utils.Math.minOptInt(accum, partition.firstEventBlockNumber) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Evaluate each partition's firstEventBlockNumber
| let getFirstEventBlockNumber = (chainFetcher: t) => | ||
| Utils.Math.minOptInt( | ||
| chainFetcher.dbFirstEventBlockNumber, | ||
| chainFetcher.fetchState->PartitionedFetchState.getFirstEventBlockNumber, | ||
| ) |
There was a problem hiding this comment.
Evaluate the DBs first event block number against the partions first event block number in the case of an indexer restart.
DZakh
left a comment
DZakh
left a comment
There was a problem hiding this comment.
Besides the newFetchedEvents->Utils.List.getLast line everything looks good 👍
And I don't like the list usage, but it's not related to the PR
| let rec getLast = (list, ~head=?) => | ||
| switch list { | ||
| | list{} => head | ||
| | list{head, ...tail} => tail->getLast(~head) | ||
| } |
There was a problem hiding this comment.
It's not for the PR, but I'd like to avoid the usage of list for batch events.
- Array is a flexible enough data structure
- List has an overhead in the runtime representation https://rescript-lang.org/try?version=v11.1.3&code=DYUwLgBAhhC8HAJYGcwG8CMAaCAmHAzDgCw4CsAvgFBA
- And it's more limiting in terms of some operations like this one
There was a problem hiding this comment.
Hey @DZakh, I agree array is generally better. Lets have a chat, some time there's trade off in both. Array concats are quite heavy for immutable operation and other types of traversal that currently happen.
Some places can probably moved back to array.
| ) => { | ||
| let firstEventBlockNumber = switch self.firstEventBlockNumber { | ||
| | Some(n) => Some(n) | ||
| | None => newFetchedEvents->Utils.List.getLast->Option.map(v => v.blockNumber) |
There was a problem hiding this comment.
Why do you get last here instead of the first?
There was a problem hiding this comment.
Ah good catch sorry I had it backwards in my head!
|
@DZakh I've updated the code for getting the list item. I had it backwards in my head (was writing for an ordering of latest to earliest event not earliest first which is the correct) |
|
Looks good 👌 |
2 participants
There was a bug introduced with partitioned fetchState where the firstEventBlockNumber can be set to any partitions first block.