-
Notifications
You must be signed in to change notification settings - Fork 622
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
refactor: use bytesize::ByteSize for TrieCacheConfig #10412
Conversation
468473b
to
4f64ad3
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #10412 +/- ##
==========================================
+ Coverage 71.91% 71.92% +0.01%
==========================================
Files 719 719
Lines 144914 144936 +22
Branches 144914 144936 +22
==========================================
+ Hits 104211 104246 +35
+ Misses 35932 35909 -23
- Partials 4771 4781 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
LGTM, always happy to see stronger types in nearcore 🎉
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.
LGTM
Just sanity check - are there any config compatibility issues (as in would this break any existing config.json that some nodes may have deployed)?
ByteSize parses integers as though they were a number of bytes, so I don’t see any potential for compatibility issues there :) |
core/store/src/trie/config.rs
Outdated
pub(crate) const DEFAULT_SHARD_CACHE_TOTAL_SIZE_LIMIT: u64 = | ||
if cfg!(feature = "no_cache") { 1 } else { 50_000_000 }; | ||
pub(crate) const DEFAULT_SHARD_CACHE_TOTAL_SIZE_LIMIT: bytesize::ByteSize = | ||
if cfg!(feature = "no_cache") { bytesize::ByteSize::mb(1) } else { bytesize::ByteSize::mb(50) }; |
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.
Is the 1B -> 1MB intentional for the no cache case?
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.
ah, that was a mistake, thanks for catching!
As suggested in #10409 (comment) to keep it consistent with the rest of
StoreConfig
.