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

fix: limit DB query result count in chunk and batch proposer #878

Merged
merged 12 commits into from
Aug 25, 2023

Conversation

Thegaram
Copy link
Contributor

Purpose or design rationale of this PR

Describe your change. Make sure to answer these three questions: What does this PR do? Why does it do it? How does it do it?

Problem: If there are many "unchunked blocks" or "unbatched chunks" in DB, chunk-proposer or batch-proposer will become extremely slow, since it will try to read all entries into memory without limit. This might also lead to memory issues.

Solution: Limit the number of entries that we read in each DB query.

PR title

Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:

  • fix: A bug fix

Deployment tag versioning

Has tag in common/version.go been updated?

  • No, this PR doesn't involve a new deployment, git tag, docker image tag
  • Yes

Breaking change label

Does this PR have the breaking-change label?

  • No, this PR is not a breaking change
  • Yes

@codecov
Copy link

codecov bot commented Aug 25, 2023

Codecov Report

Merging #878 (8007f82) into develop (6631569) will decrease coverage by 0.10%.
The diff coverage is 33.33%.

❗ Current head 8007f82 differs from pull request most recent head 3a8ca75. Consider uploading reports for the commit 3a8ca75 to get more accurate results

@@             Coverage Diff             @@
##           develop     #878      +/-   ##
===========================================
- Coverage    51.28%   51.19%   -0.10%     
===========================================
  Files           60       60              
  Lines         5588     5602      +14     
===========================================
+ Hits          2866     2868       +2     
- Misses        2483     2491       +8     
- Partials       239      243       +4     
Flag Coverage Δ
bridge 63.28% <33.33%> (-0.23%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
bridge/internal/config/config.go 68.42% <25.00%> (-31.58%) ⬇️
bridge/internal/orm/chunk.go 68.49% <25.00%> (-1.44%) ⬇️
bridge/internal/orm/l2_block.go 49.04% <25.00%> (-0.96%) ⬇️
...idge/internal/controller/watcher/batch_proposer.go 62.50% <100.00%> (ø)
...idge/internal/controller/watcher/chunk_proposer.go 67.85% <100.00%> (ø)

colinlyguo
colinlyguo previously approved these changes Aug 25, 2023
bridge/internal/controller/watcher/batch_proposer.go Outdated Show resolved Hide resolved
georgehao
georgehao previously approved these changes Aug 25, 2023
0xmountaintop
0xmountaintop previously approved these changes Aug 25, 2023
colinlyguo
colinlyguo previously approved these changes Aug 25, 2023
@Thegaram Thegaram merged commit 87f18ef into develop Aug 25, 2023
@Thegaram Thegaram deleted the db-limits-for-chunks-and-batches branch August 25, 2023 10:04
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.

4 participants