-
Notifications
You must be signed in to change notification settings - Fork 911
GODRIVER-1489 Implement streaming heartbeat protocol #423
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-1489 Implement streaming heartbeat protocol #423
Conversation
351bc7a
to
3615433
Compare
a35b9bb
to
d387c1d
Compare
The most recent commit removes the code that updated the topology based on application connection handshakes. The SDAM spec actually says drivers SHOULD use handshake responses for application connections to update the topology (https://github.com/mongodb/specifications/blob/master/source/server-discovery-and-monitoring/server-monitoring.rst#clients-update-the-topology-from-each-handshake), but most drivers don't do this and it requires additional complexity to update the RTT monitor as well. @ShaneHarvey @jyemin Thoughts on making that a "MUST NOT" in the spec instead? |
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.
I agree drivers should not update the topology from (successful) application connection handshakes. I just opened https://jira.mongodb.org/browse/DRIVERS-1300.
d68b09b
to
905a864
Compare
I did a large rebase to get the changes from some connection pool-related bugfixes and also GODRIVER-1572 (reduce race conditions in SDAM error handling). Confirmed tests pass locally, but any Evergreen failures could be related to that. |
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 with spelling fix
d8deb25
to
88ed804
Compare
88ed804
to
7577bed
Compare
This includes the changes for reducing race conditions in SDAM error handling because that work hasn't been merged into master. Once GODRIVER-1572 is closed, I'll rebase and force-push to this PR.