Skip to content

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

Merged
merged 23 commits into from
Jul 9, 2020

Conversation

divjotarora
Copy link
Contributor

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.

@divjotarora divjotarora force-pushed the streamable-ismaster-impl branch from 351bc7a to 3615433 Compare June 11, 2020 20:02
@divjotarora divjotarora requested a review from ShaneHarvey June 16, 2020 16:09
@divjotarora divjotarora force-pushed the streamable-ismaster-impl branch from a35b9bb to d387c1d Compare June 16, 2020 21:56
@divjotarora
Copy link
Contributor Author

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?

Copy link
Member

@ShaneHarvey ShaneHarvey left a 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.

@divjotarora divjotarora force-pushed the streamable-ismaster-impl branch 2 times, most recently from d68b09b to 905a864 Compare June 26, 2020 16:19
@divjotarora
Copy link
Contributor Author

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.

@jyemin jyemin removed their request for review June 30, 2020 15:44
Copy link
Contributor

@iwysiu iwysiu left a 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

@divjotarora divjotarora force-pushed the streamable-ismaster-impl branch from d8deb25 to 88ed804 Compare July 1, 2020 19:59
Divjot Arora added 2 commits July 8, 2020 14:51
@divjotarora divjotarora force-pushed the streamable-ismaster-impl branch from 88ed804 to 7577bed Compare July 8, 2020 18:55
@divjotarora divjotarora merged commit 283326f into mongodb:master Jul 9, 2020
@divjotarora divjotarora deleted the streamable-ismaster-impl branch July 9, 2020 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants