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
Added config option max_per_page #1577
Conversation
Thank you for the PR. We are fine with making these configurable as a server flag. However, we will have to run some benchmarks to understand whether making these parameters a variable ends up disturbing any compiler optimizations that are currently relying on them being static. We're in the last leg of the 0.26 release, so will test and benchmark this change in a few weeks. |
* Added max_per_page in cofig * added default topster size config * removing const default_topster_size * typo fix * Update tests.yml * few test fixes * bug fix * test fix * added couple missing lines * last test fix
* Added max_per_page in cofig * added default topster size config * removing const default_topster_size * typo fix * Update tests.yml * few test fixes * bug fix * test fix * added couple missing lines * last test fix * few fixes after updates from origin
Happy to review this now that the v26 release is done. Can you please rebase this PR against the v27 branch? Thank you. |
@kishorenc Done |
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 great, just a couple of minor changes and I can merge after that. The most important one is using uint32_t
instead of int
everywhere for max per page variable.
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.
Thank you!
Change Summary
Added config option max-per-page and default-topster-size to be able to increase max hits count per page.
Configs max-per-page and default-topster-size value should be the same.
PR Checklist
#780