Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Log provider edge cases - combined changes #13527

Closed
wants to merge 10 commits into from

Conversation

ferglor
Copy link
Collaborator

@ferglor ferglor commented Jun 12, 2024

This PR is the complete changeset introduced across the following three PRs:

Additional changes have been made on top of these three base PRs, but to summarise:

  • We are now calculating the number of iterations needed to dequeue logs for all upkeeps in the buffer, and we dequeue a different set of upkeeps in every iteration
  • We're also using a new dequeue coordinator to determine which block window should be dequeued from. The dequeue coordinator operates as follows:
    • Dequeue the windows for which we have not yet dequeued the minimum number of guaranteed logs for, going from the oldest window to the newest until all windows have had the minimum number of guaranteed logs dequeued
    • Dequeue the remaining logs from all windows as best effort, going from the oldest window to the newest, until all windows are completely dequeued
  • When a reorg has been identified, the dequeue coordinator gets reset for the affected block windows

Testing

Unit testing

Multiple unit tests have been added around the provider, buffer, and dequeue coordinator. The provider tests cover multiple scenarios regarding block progression, how dequeues are distributed across upkeeps and within block windows, as well as reorgs and changes in the number of upkeeps.

Happy path tests (AUTO-9176)

  • AUTO-8216 - 0.3% missed events vs 0.1% missed events in develop
  • AUTO-8219 - 84% missed events vs 95% missed events in develop
  • AUTO-8220 - 92% missed events vs 95% missed events in develop

@ferglor ferglor requested a review from a team as a code owner June 12, 2024 13:11
@@ -105,7 +106,11 @@ func (b *logBuffer) Enqueue(uid *big.Int, logs ...logpoller.Log) (int, int) {
b.setUpkeepQueue(uid, buf)
}

latestLogBlock, uniqueBlocks := blockStatistics(logs...)
latestLogBlock, uniqueBlocks, reorgBlocks := b.blockStatistics(logs...)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now that this function accesses and changes buffer state, it seems we need to acquire a lock

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

discussed offline, we'll need to combine lock for blockStatistics and evictReorgdLogs (maybe combine those)
also would be good to think through concurrency of the whole function

@ferglor ferglor force-pushed the feature/log-provider-edge-cases branch from e72f895 to 35f9597 Compare June 12, 2024 22:46
@ferglor ferglor force-pushed the feature/log-provider-edge-cases branch from 35f9597 to 918e748 Compare June 12, 2024 23:36
@ferglor ferglor force-pushed the feature/log-provider-edge-cases branch from 0f0bc07 to 52fd633 Compare June 13, 2024 12:32
@ferglor ferglor force-pushed the feature/log-provider-edge-cases branch from 96ffbca to 6586579 Compare June 13, 2024 21:48
@ferglor ferglor force-pushed the feature/log-provider-edge-cases branch from 6586579 to aba10a7 Compare June 13, 2024 21:52
@ferglor ferglor force-pushed the feature/log-provider-edge-cases branch from aba10a7 to 6966c69 Compare June 13, 2024 22:02
@ferglor ferglor force-pushed the feature/log-provider-edge-cases branch from 6966c69 to dad9f04 Compare June 16, 2024 07:16
amirylm
amirylm previously approved these changes Jun 20, 2024
@ferglor ferglor force-pushed the feature/log-provider-edge-cases branch from 6c0f550 to 65eb2f4 Compare June 21, 2024 12:53
@cl-sonarqube-production
Copy link

@ferglor ferglor closed this Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants