-
Notifications
You must be signed in to change notification settings - Fork 38
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
LP2053031: Adding tuning params #213
base: main
Are you sure you want to change the base?
LP2053031: Adding tuning params #213
Conversation
Hello @FrancescoDeSimone, I'm not sure if you saw the reply on Matrix, that's why I'll leave my comment here as well. Please refer here for further discussion. |
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 so much for this PR and bringing attention to it. I hope to start a good conversation around this bug. So far the work looks good i only have a problem with the default for snapshot_count
snapshot_cout now accept a string that cloud be an integer number or auto
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.
Great work, thanks a lot @FrancescoDeSimone! Left some comments and suggestions.
Co-authored-by: Homayoon Alimohammadi <[email protected]>
Co-authored-by: Homayoon Alimohammadi <[email protected]>
Co-authored-by: Homayoon Alimohammadi <[email protected]>
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.
Only a tiny nit remains. Really nice work @FrancescoDeSimone
can you check out the unit test failures?
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.
Beautiful work. LGTM 🍰
odd... the first run of the integration tests failed to complete I'll run again and see if it is successful |
@FrancescoDeSimone well I guess it wasn't a fluke. The integration tests are failing in |
this PR add tuning parameters like heartbeat_interval, election_timeout, snapshot_count, as describe in LP:#2053031