-
Notifications
You must be signed in to change notification settings - Fork 311
Disable packet multiplexing design by default #3537
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
base: main
Are you sure you want to change the base?
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.
Pull Request Overview
This PR disables the packet multiplexing design by default by enabling two AppContext switches that maintain compatibility with the previous behavior. The change addresses issues with the new packet multiplexing functionality that is not yet ready for production use.
- Enables
UseCompatibilityProcessSni
andUseCompatibilityAsyncBehaviour
switches by default - Updates logic to default to
true
when switches are not explicitly set by users - Updates tests and comments to reflect the new default behavior
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
LocalAppContextSwitches.cs | Changes default values for compatibility switches from false to true when not explicitly set |
LocalAppContextSwitchesTest.cs | Updates unit tests to expect true as default values for the compatibility switches |
MultiplexerTests.cs | Updates test helper method to return true as default when switch is not set |
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs
Outdated
Show resolved
Hide resolved
LGTM |
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.
Looks reasonable to me.
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
Overtaken by #3556 |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3537 +/- ##
==========================================
- Coverage 69.14% 67.34% -1.80%
==========================================
Files 276 274 -2
Lines 62414 62478 +64
==========================================
- Hits 43154 42075 -1079
- Misses 19260 20403 +1143
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
Enables below App Context switches by default until packet multiplexing is ready for production use.
Switch.Microsoft.Data.SqlClient.UseCompatibilityProcessSni
Switch.Microsoft.Data.SqlClient.UseCompatibilityAsyncBehaviour
Disabling feature on main branch until a decision is made to fix the feature design and implementation.