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

ledger: add pebbledb support #5606

Merged
merged 10 commits into from
Aug 9, 2023
Merged

Conversation

icorderi
Copy link
Contributor

@icorderi icorderi commented Jul 25, 2023

Summary

We are breaking down #5212 in several smaller PRs to make it more digestible.

This is PRs "numero 4" on the KV epic.

Content:

Test Plan

Existing tests.

@codecov
Copy link

codecov bot commented Jul 25, 2023

Codecov Report

Merging #5606 (a782c10) into master (259eb32) will decrease coverage by 0.02%.
Report is 7 commits behind head on master.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #5606      +/-   ##
==========================================
- Coverage   54.97%   54.95%   -0.02%     
==========================================
  Files         463      463              
  Lines       64494    64505      +11     
==========================================
- Hits        35456    35450       -6     
- Misses      26655    26674      +19     
+ Partials     2383     2381       -2     
Files Changed Coverage Δ
daemon/algod/server.go 4.14% <0.00%> (-0.16%) ⬇️
ledger/ledger.go 68.92% <0.00%> (-0.55%) ⬇️

... and 9 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@icorderi icorderi requested a review from AlgoAxel July 25, 2023 18:21
ledger/ledger.go Outdated Show resolved Hide resolved
Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

Looks good, two concerns about the hardocded configuration.

  1. max open files should be lowered and the formula in server.go updated if driver="pebble" is selected.
  2. 4GB per mem table is too high for npn

Additionally, please comment, modifying of which config options might lead to performance issues for existing pebble DBs.

@icorderi
Copy link
Contributor Author

icorderi commented Aug 7, 2023

@algorandskiy the default config is for 32 MB per memtable, with 2 memtables, not 4 GB.

@algorandskiy
Copy link
Contributor

ah,

	maxMemTableSize := 4<<30 - 1 // Capped by 4 GB

this is max, not actual, ok

@algorandskiy algorandskiy changed the title feat: add pebbledb support ledger: add pebbledb support Aug 8, 2023
algorandskiy
algorandskiy previously approved these changes Aug 8, 2023
AlgoAxel
AlgoAxel previously approved these changes Aug 9, 2023
Copy link
Contributor

@AlgoAxel AlgoAxel left a comment

Choose a reason for hiding this comment

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

Left you a comment on an error "string", and also would still like to hear more about which of these pebble db options we might have trouble changing in the future.

daemon/algod/server.go Outdated Show resolved Hide resolved
ledger/ledger.go Show resolved Hide resolved
daemon/algod/server.go Outdated Show resolved Hide resolved
@algorandskiy algorandskiy dismissed stale reviews from AlgoAxel and themself via a782c10 August 9, 2023 17:55
@algorandskiy algorandskiy merged commit a36464a into algorand:master Aug 9, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants