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

Core pooling logic #2686

Merged

mdaigle merged 28 commits into dotnet:feat/sqlclientx from mdaigle:pooling-internals

Jul 25, 2024

Contributor

mdaigle commented Jul 18, 2024 •

edited

Loading

This PR covers core pooling logic including SqlConnector creation, retrieval, return, and statistics. Integrates with rate limiter scaffolding added in #2667. Also covers small fixes for create, close, and dispose flows exposed by unit tests.

Not included in this PR are:

warmup
pruning
clearing the pool
data source disposal
connection lifetime evaluation
metrics and reporting
exception patterns in line with the rest of the driver

mdaigle added 11 commits

July 16, 2024 16:22


          Pull over rough outline from npgsql

6abf233


          Pull over pool tests. Does not compile yet.

a470af5


          Set up test structure. Refactors for visibility for testing.

e544f28


          Implement disposal. Set ConnectionString property.

f228114


          Disable pool blocking for tests. Pull over internal connection open l…

1cd0a58

…ogic.


          Implement return in pool. Clean up return flow in connection objects.

5bd5538


          Fix CreateConnection method in SqlDataSource.

8124da3


          Adjust timeout exception.

5cda5d1


          Add data source statistics. Fix connection sync dispose to invoke clo…

16d196b

…se and release internal connection to pool.


          Enable exercise test.

7ad8c2b


          Merge branch 'feat/sqlclientx' into pooling-internals

3f7ce7e

edwardneal reviewed

View reviewed changes

Contributor

edwardneal left a comment

I think this matches our discussion about pooling (pending the missing parts you've already noted.) There are a few points of code formatting/indentation/resource strings, but nothing worth a specific comment until the PR's out of draft. I'm happy with the base methodology (besides one or two nits in review comments.)

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClientX/PoolingDataSource.cs Outdated Show resolved Hide resolved

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClientX/PoolingDataSource.cs Outdated Show resolved Hide resolved

mdaigle added 5 commits

July 19, 2024 10:18


          Add license statements.

47f8431


          Review changes.

9c16cdb


          Merge branch 'pooling-internals' of github.com:mdaigle/SqlClient into…

c5a557a

… pooling-internals


          Add comments and adjust method visibility.

c354c1e


          Remove unused using statements.

11d63ab

mdaigle marked this pull request as ready for review

July 19, 2024 21:57

mdaigle changed the title ~~Pooling internals~~ Core pooling logic

benrr101 requested changes

View reviewed changes

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClientX/PoolingDataSource.cs Outdated Show resolved Hide resolved

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClientX/PoolingDataSource.cs Outdated Show resolved Hide resolved

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClientX/PoolingDataSource.cs Outdated Show resolved Hide resolved

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClientX/PoolingDataSource.cs Show resolved Hide resolved

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClientX/PoolingDataSource.cs Outdated Show resolved Hide resolved

src/Microsoft.Data.SqlClient/tests/UnitTests/PoolTests.cs



		[Fact]
		public async Task MinPoolSize_equals_MaxPoolSize()

Contributor

benrr101 Jul 22, 2024

Just going to suggest my philosophy of unit test organization - organizing them into Arrange, Act, Assert is a fairly standard practice that helps make it clear what's being tested.

src/Microsoft.Data.SqlClient/tests/UnitTests/PoolTests.cs

+                      }
+                      [Fact]
+                      public void MinPoolSize_bigger_than_MaxPoolSize_throws()

Contributor

benrr101 Jul 22, 2024

Also usually naming the tests like MethodName_OptionalCondition_Outcome is a bit more readable
Eg, for this one I'd rename it MinPoolSize_BiggerThanMaxPoolSize_Throws

src/Microsoft.Data.SqlClient/tests/UnitTests/PoolTests.cs Outdated Show resolved Hide resolved

src/Microsoft.Data.SqlClient/tests/UnitTests/PoolTests.cs

+                          };
+                          // Start an async open which will not complete as the pool is exhausted.
+                          var asyncOpenerTask = asyncOpener();

Contributor

benrr101 Jul 22, 2024

I'm a bit confused on this one... Would this be more simply expressed using Task.Run?

src/Microsoft.Data.SqlClient/tests/UnitTests/PoolTests.cs Outdated Show resolved Hide resolved

mdaigle added 3 commits

July 22, 2024 16:09


          Adding braces.

0d9b53a


          Clean up loop syntax.

394db3c


          Address review comments.

e8896a2

benrr101 approved these changes

View reviewed changes

Contributor

benrr101 left a comment

A few low-tier commends, but nothing that I'd hold up checkin in on. I think.

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClientX/PoolingDataSource.cs Outdated Show resolved Hide resolved

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClientX/PoolingDataSource.cs Outdated Show resolved Hide resolved

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClientX/SqlConnector.cs Outdated Show resolved Hide resolved


          Add sleep to exercise test.

937a0c8

saurabh500 reviewed

View reviewed changes

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClientX/PoolingDataSource.cs Outdated Show resolved Hide resolved

mdaigle added 3 commits

July 23, 2024 15:19


          Review changes.

fdccf10


          Adjust signature.

d292818


          Merge branch 'feat/sqlclientx' into pooling-internals

264af95

saurabh500 reviewed

View reviewed changes

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClientX/PoolingDataSource.cs Outdated Show resolved Hide resolved

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClientX/PoolingDataSource.cs Outdated Show resolved Hide resolved

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClientX/SqlConnectionX.cs Outdated Show resolved Hide resolved

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClientX/SqlConnectionX.cs Show resolved Hide resolved

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClientX/ThrowHelper.cs Outdated Show resolved Hide resolved

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClientX/SqlConnector.cs Show resolved Hide resolved

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClientX/PoolingDataSource.cs Outdated Show resolved Hide resolved

mdaigle added 5 commits

July 24, 2024 11:58


          Clean up commented code.


          Merge branch 'pooling-internals' of github.com:mdaigle/SqlClient into…

f6da3f3

… pooling-internals


          Shift wait to connector.

c8ba462


          Use SqlConnectionString instead of builder.

fdd3abc


          Add attribution statement.

75e5b3b

mdaigle merged commit a97e2f1 into dotnet:feat/sqlclientx

11 checks passed

mdaigle deleted the pooling-internals branch

July 25, 2024 20:39

This was linked to issues Jul 25, 2024

Implement idle connection queue and connection retrieval for connection pool #2627

Closed

Implement connection opening in connection pool #2628

Closed

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

Labels

None yet