Last indexed cleanup scribe/agents testing optimization #1409
Conversation
WalkthroughThe changes primarily focus on improving the accuracy of tracking the last indexed block in the Scribe service. A new variable Changes
TipsChat with CodeRabbit Bot (
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1409 +/- ##
===================================================
+ Coverage 50.51623% 51.15537% +0.63913%
===================================================
Files 343 366 +23
Lines 24117 25057 +940
Branches 277 267 -10
===================================================
+ Hits 12183 12818 +635
- Misses 10724 10966 +242
- Partials 1210 1273 +63
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (1)
- services/scribe/service/indexer/indexer.go (2 hunks)
Additional comments (Suppressed): 1
services/scribe/service/indexer/indexer.go (1)
- 229-241: The new logic ensures that the last indexed value is only updated when all logs from the last block have been processed. This is a significant improvement over the previous implementation where the last indexed value was updated prematurely. The error handling and logging are also appropriately done.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (2)
- services/scribe/grpc/server/server.go (1 hunks)
- services/scribe/service/indexer/indexer.go (2 hunks)
Files skipped from review due to trivial changes (1)
- services/scribe/grpc/server/server.go
Additional comments (Suppressed): 2
services/scribe/service/indexer/indexer.go (2)
180-186: The introduction of
lastBlockSeenvariable is a good approach to track the last block number processed by the indexer. This will help in ensuring that the last indexed value is only updated when all logs from the last block have been processed, preventing premature updates. However, it's important to ensure that this change doesn't introduce any race conditions or synchronization issues if multiple goroutines are accessing and modifying this variable concurrently.229-241: The modification to the
saveLastIndexedfunction call is well thought out. By checking iflastBlockSeen < log.BlockNumberbefore updating the last indexed block, you're ensuring that all logs from the last block have been processed. This prevents premature updates of the last indexed block and reduces complexity for downstream dependencies. However, please verify that there are no side effects due to this change in other parts of the codebase that rely on thesaveLastIndexedfunction.- err = x.saveLastIndexed(storeCtx, log.BlockNumber) + if lastBlockSeen < log.BlockNumber { + err = x.saveLastIndexed(storeCtx, lastBlockSeen) + if err != nil { + logger.ReportIndexerError(err, x.indexerConfig, logger.StoreError) + return fmt.Errorf("could not store last indexed: %w", err) + } + lastBlockSeen = log.BlockNumber + }
nautsimon
changed the title
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (2)
- services/scribe/config/config.go (1 hunks)
- services/scribe/service/scribe.go (1 hunks)
Files skipped from review due to trivial changes (1)
- services/scribe/config/config.go
Additional comments (Suppressed): 4
services/scribe/service/scribe.go (4)
85-85: The error message has been updated to include the error from the chain context cancellation. This provides more detailed information about the error, which can be useful for debugging.
92-92: The error message has been updated to include the error from the chain indexer. This provides more detailed information about the error, which can be useful for debugging.
85-85: Ensure that the updated error message does not break any error handling that relies on the specific text of the old error message.
92-92: Ensure that the updated error message does not break any error handling that relies on the specific text of the old error message.
Description
The long standing behavior of last indexed has been that it is mutated whenever a log is seen and processed on that block. This behavior spawned a workaround in the gRPC server where the last block indexed lagged one block behind the last block stored in scribe. This workaround spawned more workarounds in the agents test suite where the simulated backends had to bump their head blocks by one to retrieve all data from scribe.
Last indexed should transmit the last indexed block, if there are still logs that have not been indexed in that block, then that block should not be considered the last indexed block. This PR fixes this.
Remedying this behavior will clean up testing and will reduce complexity on downstream dependancies (agents testing suite).
Summary by CodeRabbit
Release Notes:
lastBlockSeenvariable inindexer.goto improve the accuracy of tracking the last indexed block.toBlockvariable assignment inserver.goto ensure it is set to the latest value oflatestScribeBlock.Verbosefield inconfig.goto enable verbose logging.scribe.goby adding error messages for chain context cancellation and chain indexer errors.cmd.mdfor clarity.