-
Notifications
You must be signed in to change notification settings - Fork 102
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 rounding function for max entries #1381
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1381 +/- ##
=======================================
Coverage 81.09% 81.09%
=======================================
Files 147 147
Lines 14745 14751 +6
=======================================
+ Hits 11957 11962 +5
- Misses 2205 2206 +1
Partials 583 583
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
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.
Shoud we add unit test for this?
@marctc good idea, will do |
Do not round to 0, round to at least N
975fa6a
to
3dfa1e5
Compare
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! But it made me think, shouldn't we always round up? Rounding down can lead to unexpected behaviour.
@grcevski you are right - but there are also issues with rounding up, namely excessive memory consumption - for instance, if the page size is 16384 and we have the hypothetical value of 17000, it would round up to 32768, which would be super wasteful - so that's the trade-off. I will merge the PR but happy to raise a follow up if it turns out rounding up is still the best approach. |
Do not round to 0, round to at least N