-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
fix: session support detection spec compliance #2732
Conversation
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
test/unit/core/common.js
Outdated
}); | ||
|
||
this.defineReplSetStates(); | ||
// this.configureMessageHandlers(); |
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.
Should this line be commented out? Seems there are some issues in CI.
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.
Whoops good catch, let's see if the commit I just pushed fixes it. I made the change on the 4.0 port already to add if statements to gate these extra setup calls.
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 👍
Discovered while working on a help ticket the reduce callback in the topology description
doesn't correctly return null if it's in the middle of the list of servers.