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.

Sign up for GitHub

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

Jump to bottom

fix(NODE-5723): emit TopologyDescriptionChangedEvent just before close #3968

Open
wants to merge 34 commits into
base: main
Choose a base branch
from
Open

fix(NODE-5723): emit TopologyDescriptionChangedEvent just before close #3968

wants to merge 34 commits into from

Conversation

W-A-James
Copy link
Contributor

@W-A-James W-A-James commented Jan 17, 2024 •
edited

Description

What is changing?

  • Unskip Topology life cycle tests
  • Remove modified logging spec tests
  • Pull in updated logging-loadbalancer tests
  • Pull in new monitoring tests
  • Fix bug in test runner that was exiting on encountering a single SDAM event
  • Emit TopologyDescriptionChangedEvent transitioning to Unknown Topology type just before close
Is there new documentation needed for these changes?

No

What is the motivation for this change?

NODE-5723 / DRIVERS-2711

Release Highlight

Fill in title or leave empty for no highlight

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@W-A-James W-A-James changed the title emit TopologyDescriptionChangedEvent just before close fix(NODE-5723): emit TopologyDescriptionChangedEvent just before close Jan 17, 2024
@W-A-James
Copy link
Contributor Author

W-A-James commented Jan 18, 2024 •
edited

Note: getting spec test failures, but they disappear when only running the tests in question.

Failures were coming from old modified spec tests that have now been removed.

W-A-James
W-A-James commented Jan 19, 2024
View reviewed changes
test/manual/socks5.test.ts
@@ -302,7 +302,7 @@ describe('Socks5 Connectivity', function () {
async function testConnection(connectionString, clientOptions) {
const client = new MongoClient(connectionString, clientOptions);
let topologyType;
client.on('topologyDescriptionChanged', ev => (topologyType = ev.newDescription.type));
client.on('topologyDescriptionChanged', ev => (topologyType = ev.previousDescription.type));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function was previously relying on the fact that the last TopologyDescroptionChangedEvent would be the one transitioning the topology to the fully known, desired operating state, with the description being in the newDescription field.

Now that we emit a final instance of that event transitioning to the UNKNOWN state, this final event would have the desired topology description in the previousDescription field, rather than the newDescription field.

@W-A-James W-A-James mentioned this pull request Mar 26, 2024
Merged
4 tasks
@W-A-James
Copy link
Contributor Author

Note: this branch was rebased against #4089 which implements the UTR changes introduced in DRIVERS-2875

@W-A-James W-A-James marked this pull request as ready for review May 7, 2024 20:20
@nbbeeken nbbeeken self-assigned this May 9, 2024
@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label May 9, 2024
nbbeeken
nbbeeken requested changes May 9, 2024
View reviewed changes
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

code lgtm, one small question.

You are calling this a fix, was the previous event emission technically "wrong"? can you make your release notes describe the change in terms of what it is fixing?

src/sdam/topology.ts Outdated Show resolved Hide resolved
nbbeeken
nbbeeken requested changes May 13, 2024
View reviewed changes
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

From slack:

missed a TODO skip message to run some of the tests ... that are failing because of an extra event emission

Just keeping this here for viz.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nbbeeken nbbeeken nbbeeken requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@nbbeeken nbbeeken

Labels
Primary Review In Review with primary reviewer, not yet ready for team's eyes
Projects
None yet
Milestone
No milestone
2 participants
@W-A-James @nbbeeken