-
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
feat(NODE-5189): deprecate tcp keepalive options #3621
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.
@durran No concerns with the deprecations (as long as there is also a clear ticket to remove the options), but changing the defaults is arguably breaking and I think that's why we framed NODE-3607 as a spike; can we take this back and discuss it as a team? I didn't see a kickoff comment or a link to an external doc summarizing the results of the investigation, but some notes addressing the 6 spec callouts point by point would help figure out what changes can happen now and which ones need a follow up ticket.
src/connection_string.ts
Outdated
@@ -864,12 +864,14 @@ export const OPTIONS = { | |||
return wc; | |||
} | |||
}, | |||
/** @deprecated - Will not be able to turn off in the future. */ |
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.
We have the deprecated: true
property that we can add to the option description objects here to get warnings.
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.
Added option with message.
src/cmap/connect.ts
Outdated
// Default to delay to 300 seconds. Node automatically then sets TCP_KEEPINTVL to 1 second | ||
// which is acceptable to the recommendation of 10 seconds and also cannot be configured. | ||
// TCP_KEEPCNT is also set to 10 in Node and cannot be configured. (Recommendation is 9) | ||
const keepAliveInitialDelay = options.keepAliveInitialDelay || 300000; |
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.
const keepAliveInitialDelay = options.keepAliveInitialDelay || 300000; | |
const keepAliveInitialDelay = options.keepAliveInitialDelay; |
Because of the defaulting, we should always have a defined number here.
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.
Updated.
I've update the NODE-3607 JIRA ticket in the comments with notes for each point in the spec. |
src/cmap/connect.ts
Outdated
// Default to delay to 300 seconds. Node automatically then sets TCP_KEEPINTVL to 1 second | ||
// which is acceptable to the recommendation of 10 seconds and also cannot be configured. | ||
// TCP_KEEPCNT is also set to 10 in Node and cannot be configured. (Recommendation is 9) | ||
const keepAliveInitialDelay = options.keepAliveInitialDelay; |
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.
We only want to do the deprecations; changing the defaults would be a potentially breaking change (at any rate it would change behavior), so we can do that in NODE-5190.
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.
Removed the changes, only deprecations now.
Description
Updates TCP keepalive options to the spec where appropriate.
What is changing?
keepAlive
andkeepAliveInitialDelay
options.keepAliveInitialDelay
to 300 seconds.Node does not allow fine tuning of the keepalive options as per the spec, so we set the initial delay and then accept the other settings that Node sets internally. This results in:
SO_KEEPALIVE=1
TCP_KEEPIDLE=300000
TCP_KEEPCNT=10
TCP_KEEPINTVL=1
Is there new documentation needed for these changes?
mongodb/docs-node#639
What is the motivation for this change?
NODE-3607
Double check the following
npm run check:lint
scripttype(NODE-xxxx)[!]: description
feat(NODE-1234)!: rewriting everything in coffeescript