Conversation
ryle-ai
left a comment
There was a problem hiding this comment.
Review Summary
This PR has been open for ~9 months and now has merge conflicts with the develop branch.
Current Status
- Merge State: CONFLICTING (cannot be merged as-is)
- Last Activity: May 2025
Observations
1. Retry Logic May Be Partially Superseded
Commit 53c002b ("add retry #109") was merged to develop, which may have incorporated similar retry logic. Need to verify if this PR's retry implementation is still needed or if it duplicates existing functionality.
2. Key Concerns if Revived
If this PR is still relevant and will be rebased, please address:
-
Balance State Accumulation: The
updateBalanceMapfunction merges only changed balances, but subsequent diff comparisons need full balance state. This could cause cascading incorrect diffs. -
Inconsistency Between Full/Incremental: Full snapshots store complete balance maps, but incremental snapshots only store changes. This asymmetry may cause issues when processing alternates between snapshot types.
-
Unused
balanceStateField: The newbalanceStatefield inMetagraphDataappears to be computed but never consumed downstream. Is this intended for future use? -
Retry Mechanism: Only
EofExceptionis retried. Consider:- Adding exponential backoff
- Handling other transient database errors
- Adding delay between retries
Recommendation
Please either:
- Close this PR if the optimization is no longer needed or has been implemented differently
- Rebase against current develop and address the merge conflicts, then request re-review
Happy to do a thorough review of the updated code once conflicts are resolved.
Reviewed against snapshot-streaming codebase
ryle-ai
left a comment
There was a problem hiding this comment.
@gclaramunt just checking back in -- this has been open for a while now and the merge conflicts are still there. Is this something you're planning to pick back up, or would it make sense to close it out?
No description provided.