-
Notifications
You must be signed in to change notification settings - Fork 890
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
GODRIVER-1778 Reject HeartbeatFrequencyMS of less than 500ms #1828
Conversation
API Change ReportNo changes found! |
Possible Follow-Ups:
|
@@ -45,7 +45,7 @@ const ( | |||
) | |||
|
|||
var ( | |||
defaultHeartbeatInterval = 50 * time.Millisecond | |||
defaultHeartbeatInterval = 500 * time.Millisecond |
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.
What is the history of the original 50ms default? Does the 500ms requirement conflict with legacy test specifications? Or was this value chosen arbitrarily?
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 like it came from this commit to implement streaming heartbeat protocol, not sure how the value was chosen though.
We should document the minimum value on the options setter function. |
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, with a few suggestions! 👍
3d83d20
GODRIVER-1778
Summary
The Server Monitoring spec explicitly forbids setting a heartbeatFrequencyMS of less than 500ms:
For both multi- and single-threaded drivers, the driver MUST NOT permit users to configure it less than minHeartbeatFrequencyMS (500ms).
Background & Motivation