NotificationsYou must be signed in to change notification settings
Fork311
Star927

Refactor for modular connection pool#3199

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

mdaigle merged 10 commits intodotnet:mainfrommdaigle:modular-connection-pool

Mar 7, 2025

Merged

Refactor for modular connection pool#3199

mdaigle merged 10 commits intodotnet:mainfrommdaigle:modular-connection-pool

Mar 7, 2025

Conversation

Copy link

Contributor

mdaigle commentedMar 5, 2025•
edited
Loading

Introduces an abstract class to capture the API of the current connection pool implementation. Gathers pooling related classes into a single namespace (Microsoft.Data.SqlClient.ConnectionPool) and folder. These changes set the stage for swappable connection pool implementations. Targeting main because these are good standalone changes for code cleanliness. Future changes will target a feature branch.

This work is tracking against#25

PoolBlockingPeriod is not moved because it is public. Moving it's location/namespace would be a breaking API change. It may still make sense to switch its namespace if this can go in with a major version bump.

mdaigle added8 commits

March 4, 2025 11:49

Add abstract class to define pool interface.

eb423fd

Code cleanup

306efa5

Refactoring pool state and cross-instance access.

e2b9615

Rename files

ad1fa75

Rename abstract base back to DbConnectionPool

2cd3a58

Reorganize pool files.

a99ca38

Fix imports

7cbb506

Merge branch 'main' of github.com:dotnet/SqlClient into modular-conne…

3d34293

…ction-pool

mdaigle changed the title~~Modular connection pool~~Refactor for modular connection pool

Mar 5, 2025

Copy link

ContributorAuthor

mdaigle commentedMar 5, 2025

/azp run

Copy link

azure-pipelinesbot commentedMar 5, 2025

Azure Pipelines successfully started running 1 pipeline(s).

Fix pooling test reflection

3f434bf

Copy link

codecovbot commentedMar 6, 2025•
edited
Loading

Codecov Report

Attention: Patch coverage is84.00000% with12 lines in your changes missing coverage. Please review.

Project coverage is 65.96%. Comparing base(3f4e486) to head(b7a321e).
Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
...lient/ConnectionPool/WaitHandleDbConnectionPool.cs	82.35%	12 Missing⚠️

❗ There is a different number of reports uploaded between BASE (3f4e486) and HEAD (b7a321e). Click for more details.
HEAD has 1 upload less than BASE
Flag BASE (3f4e486) HEAD (b7a321e)
addons 1 0

Additional details and impacted files

@@            Coverage Diff             @@##             main    #3199      +/-   ##==========================================- Coverage   72.89%   65.96%   -6.94%==========================================  Files         288      283       -5       Lines       59246    59209      -37     ==========================================- Hits        43190    39055    -4135- Misses      16056    20154    +4098

Flag	Coverage Δ
addons	`?`
netcore	`68.82% <83.09%> (-6.69%)`	⬇️
netfx	`64.87% <84.00%> (-6.41%)`	⬇️

Flags with carried forward coverage won't be shown.Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report?Share it here.

🚀 New features to boost your workflow:

❄Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mdaigle changed the base branch frommain todev/mdaigle/connection-pool-perf

March 6, 2025 19:45

mdaigle marked this pull request as ready for review

March 6, 2025 19:45

mdaigle changed the base branch fromdev/mdaigle/connection-pool-perf tomain

March 6, 2025 19:47

mdaigle requested a review fromCopilot

March 6, 2025 20:06

CopilotAI reviewed

Mar 6, 2025

View reviewed changes

Copy link

Contributor

CopilotAI left a comment

Choose a reason for hiding this comment

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

PR Overview

This PR refactors the connection pool implementation to be more modular by introducing an abstract DbConnectionPool class and moving pooling-related classes into the Microsoft.Data.SqlClient.ConnectionPool namespace.

Introduces a new abstract class (DbConnectionPool) that captures the current connection pool API.
Migrates several classes from Microsoft.Data.ProviderBase to Microsoft.Data.SqlClient.ConnectionPool.
Updates pool instantiation in DbConnectionPoolGroup to use WaitHandleDbConnectionPool, supporting modular and swappable implementations.

Reviewed Changes

File	Description
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/DbConnectionPool.cs	Adds a new abstract base class for connection pooling API.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/DbConnectionPoolCounters.netfx.cs	Updates the namespace to the new connection pool namespace.
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlConnectionHelper.cs	Adapts references from the old provider base pool group to the new connection pool group.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/DbConnectionPoolIdentity.Unix.cs	Updates namespace from ProviderBase to SqlClient.ConnectionPool.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/DbConnectionPoolAuthenticationContextKey.cs	Updates namespace from ProviderBase to SqlClient.ConnectionPool.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/DbConnectionPoolGroupProviderInfo.cs	Updates namespace from ProviderBase to SqlClient.ConnectionPool.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/DbConnectionPoolAuthenticationContext.cs	Updates namespace from ProviderBase to SqlClient.ConnectionPool.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/DbConnectionPoolGroup.cs	Modifies pool instantiation to use WaitHandleDbConnectionPool.
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs	Incorporates the new connection pool namespace in factory logic.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionFactory.cs	Updates pool group references to use the new connection pool namespace.
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs	Updates pool group references to use the new connection pool namespace.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs	Adapts using statements to include the new connection pool namespace.
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlConnectionHelper.cs	Updates namespace references accordingly.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionClosed.cs	Updates namespace to use the new connection pool namespace.
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs	Updates namespace to include the new connection pool namespace.
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlConnection.cs	Updates namespace imports accordingly.
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlConnection.cs	Updates namespace imports accordingly.
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs	Updates namespace imports accordingly.

Copilot reviewed 33 out of 33 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/DbConnectionPoolGroup.cs:190

Please ensure that the new WaitHandleDbConnectionPool implementation is thoroughly tested to validate its initialization, cleanup, and overall behavior in the connection pool lifecycle.

DbConnectionPool newPool = new WaitHandleDbConnectionPool(connectionFactory, this, currentIdentity, connectionPoolProviderInfo);

benrr101 requested changes

Mar 6, 2025

View reviewed changes

Copy link

Contributor

benrr101 left a comment

Choose a reason for hiding this comment

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

Overall I like these changes. Mostly stylistic changes I'm requesting, a couple bigger questions around what the longer term plan is.

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlConnectionFactory.csShow resolvedHide resolved

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlConnectionHelper.cs OutdatedShow resolvedHide resolved

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/DbConnectionPool.cs OutdatedShow resolvedHide resolved

...oft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs OutdatedShow resolvedHide resolved

...soft.Data.SqlClient/tests/ManualTests/SQL/Common/SystemDataInternals/ConnectionPoolHelper.csShow resolvedHide resolved

Address review comments

b7a321e

mdaigle requested a review froma team

March 6, 2025 23:12

benrr101 approved these changes

Mar 7, 2025

View reviewed changes

Copy link

Contributor

benrr101 left a comment

Choose a reason for hiding this comment

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

Looks much better now, thanks!

cheenamalhotra approved these changes

Mar 7, 2025

View reviewed changes

mdaigle added this to the7.0-preview1 milestone

Mar 7, 2025

mdaigle merged commit91f238c intodotnet:main

Mar 7, 2025

123 checks passed

mdaigle deleted the modular-connection-pool branch

March 7, 2025 17:06

Labels

None yet

3 participants

Movatterモバイル変換

Refactor for modular connection pool#3199

Refactor for modular connection pool#3199

Uh oh!

Conversation

mdaigle commentedMar 5, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

mdaigle commentedMar 5, 2025

Uh oh!

azure-pipelinesbot commentedMar 5, 2025

Uh oh!

codecovbot commentedMar 6, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Codecov Report

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

PR Overview

Reviewed Changes

Uh oh!

benrr101 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benrr101 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mdaigle commentedMar 5, 2025•
edited
Loading

codecovbot commentedMar 6, 2025•
edited
Loading