Skip to content
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-2265 Make more correct assertions in SDAM heartbeat test. #837

Conversation

matthewdale
Copy link
Collaborator

GODRIVER-2265

The test TestSDAMProse/heartbeats_processed_more_frequently fails intermittently on the Evergreen waterfall. Some assertions made in that test are only accidentally working:

  • The previous assertion that server version <4.4 processes a different number of operations for all heartbeat intervals is not correct. The discrepancy between the number of operations processed between server versions <4.4 and >=4.4 comes from the "minimum heartbeat interval" of 500ms that only applies to non-streaming heartbeats.
  • The previous assertion that a Client should send a heartbeat exactly every interval is not correct. A Client waits at least the specified interval between heartbeats, and does not guarantee exactly when the next heartbeat will run (the test asserts that heartbeats can occur at up to 2 times the specified interval).

Correct the assertions made in the TestSDAMProse/heartbeats_processed_more_frequently test and hopefully improve the reliability of the test in some circumstances.

Copy link
Contributor

@benjirewis benjirewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating this test! If your assertions about the behavior of heartbeatInterval are correct, this looks good. Is heartbeatInterval described as a rate-limiting option (the Client will wait at least 500ms between heartbeats) somewhere in the spec?

mongo/integration/sdam_prose_test.go Show resolved Hide resolved
@matthewdale
Copy link
Collaborator Author

The statement

the Client will wait at least 500ms between heartbeats

is based on the behavior of timers in Go, not based on the spec. For example, the description of time.NewTimer says it

will send the current time on its channel after at least duration d

Basically, Go timers don't guarantee exactly when something will happen, only that it won't happen before a specific time. The previous test attempted to assert behavior closer to exact timing, but failed when the Go runtime couldn't schedule those heartbeats exactly on time (maybe because the Evergreen host CPU was busy).

There is a rate-limit described in the spec, minHeartbeatFrequencyMS.

If a client frequently rechecks a server, it MUST wait at least minHeartbeatFrequencyMS milliseconds since the previous check ended

That describes a similar behavior to how timers in Go work, so they're actually both relevant, but my original intent was to express the limitation of Go timers.

Copy link
Contributor

@benjirewis benjirewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, thanks for the explanation. Looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants