-
Notifications
You must be signed in to change notification settings - Fork 470
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
Conversation
Codecov Report
@@ 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
... and 9 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this 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.
- max open files should be lowered and the formula in server.go updated if driver="pebble" is selected.
- 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.
@algorandskiy the default config is for 32 MB per memtable, with 2 memtables, not 4 GB. |
ah,
this is max, not actual, ok |
There was a problem hiding this 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.
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.