Skip to content
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

pool/SNOW-902608 #2/4 new pool version v2 #794

Conversation

sfc-gh-mhofman
Copy link
Collaborator

@sfc-gh-mhofman sfc-gh-mhofman commented Oct 11, 2023

Description

Connection Pool v2

  • introduction of connection manager handling multiple pool of sessions
  • ability to switch Connection Manager between v1 and v2
  • controlling pool settings for each pool independently
  • default pool version: v1

Checklist

  • Code compiles correctly
  • Code is formatted according to Coding Conventions
  • Created tests which fail without the change (if possible)
  • All tests passing (dotnet test)
  • Extended the README / documentation, if necessary
  • Provide JIRA issue id (if possible) or GitHub issue id in PR name

@codecov
Copy link

codecov bot commented Oct 11, 2023

Codecov Report

Merging #794 (c401f2c) into pool/SNOW-860872-connection-pool (22d32dd) will increase coverage by 0.24%.
The diff coverage is 95.36%.

@@                         Coverage Diff                          @@
##           pool/SNOW-860872-connection-pool     #794      +/-   ##
====================================================================
+ Coverage                             83.56%   83.80%   +0.24%     
====================================================================
  Files                                    89       91       +2     
  Lines                                  9155     9313     +158     
  Branches                                837      857      +20     
====================================================================
+ Hits                                   7650     7805     +155     
- Misses                                 1278     1279       +1     
- Partials                                227      229       +2     
Files Coverage Δ
Snowflake.Data/Core/SFError.cs 100.00% <ø> (ø)
...wflake.Data/Core/Session/ConnectionCacheManager.cs 100.00% <100.00%> (ø)
Snowflake.Data/Core/Session/SFSession.cs 79.79% <100.00%> (+0.37%) ⬆️
Snowflake.Data/Core/Session/SessionFactory.cs 100.00% <100.00%> (ø)
Snowflake.Data/Core/Session/SessionPool.cs 90.00% <97.05%> (+2.64%) ⬆️
Snowflake.Data/Client/SnowflakeDbConnectionPool.cs 95.23% <92.30%> (-4.77%) ⬇️
...owflake.Data/Core/Session/ConnectionPoolManager.cs 95.91% <95.91%> (ø)

... and 2 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@sfc-gh-mhofman sfc-gh-mhofman changed the title Pool/snow 902608 new pool version Pool/SNOW-902608 #2/4 new pool version Oct 12, 2023
@sfc-gh-mhofman sfc-gh-mhofman changed the title Pool/SNOW-902608 #2/4 new pool version Pool/SNOW-902608 #2/4 new pool version v2 Oct 12, 2023
@sfc-gh-mhofman sfc-gh-mhofman changed the title Pool/SNOW-902608 #2/4 new pool version v2 pool/SNOW-902608 #2/4 new pool version v2 Oct 16, 2023
@sfc-gh-mhofman sfc-gh-mhofman force-pushed the pool/SNOW-902608-new-pool-version branch 6 times, most recently from 0612dc5 to 55a764a Compare October 17, 2023 09:59
Snowflake.Data/Client/SnowflakeDbConnectionPool.cs Outdated Show resolved Hide resolved
Snowflake.Data/Core/Session/ConnectionPoolManager.cs Outdated Show resolved Hide resolved
Snowflake.Data/Core/Session/ConnectionPoolManager.cs Outdated Show resolved Hide resolved
Snowflake.Data.Tests/Util/PoolConfig.cs Outdated Show resolved Hide resolved
Snowflake.Data/Core/Session/SessionPool.cs Outdated Show resolved Hide resolved
Snowflake.Data/Client/SnowflakeDbConnectionPool.cs Outdated Show resolved Hide resolved
Snowflake.Data/Core/Session/ConnectionPoolManager.cs Outdated Show resolved Hide resolved
Snowflake.Data/Core/Session/ConnectionPoolManager.cs Outdated Show resolved Hide resolved
Snowflake.Data/Core/Session/ConnectionPoolManager.cs Outdated Show resolved Hide resolved
@sfc-gh-mhofman sfc-gh-mhofman marked this pull request as ready for review October 31, 2023 15:14
@sfc-gh-mhofman sfc-gh-mhofman requested a review from a team as a code owner October 31, 2023 15:14
@sfc-gh-mhofman sfc-gh-mhofman force-pushed the pool/SNOW-902608-new-pool-version branch from 5d4c5af to 259e67a Compare October 31, 2023 16:01
Snowflake.Data/Core/Session/ConnectionCacheManager.cs Outdated Show resolved Hide resolved
Snowflake.Data/Core/Session/ConnectionPoolManager.cs Outdated Show resolved Hide resolved
Snowflake.Data/Core/Session/SessionPool.cs Outdated Show resolved Hide resolved
Snowflake.Data/Core/Session/SessionPool.cs Outdated Show resolved Hide resolved
Snowflake.Data/Core/Session/SessionPool.cs Outdated Show resolved Hide resolved
Snowflake.Data/Core/Session/SFSession.cs Outdated Show resolved Hide resolved
@sfc-gh-mhofman
Copy link
Collaborator Author

The code is inactive and there is no public api to enable the new pool for now.
Hence, once approved we can merge and wait for security review to be completed in the background.

Copy link
Collaborator

@sfc-gh-dstempniak sfc-gh-dstempniak left a comment

Choose a reason for hiding this comment

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

LGTM

@sfc-gh-mhofman sfc-gh-mhofman force-pushed the pool/SNOW-902608-new-pool-version branch from 5f71f0e to d309552 Compare November 9, 2023 13:06
- removed singleton pattern from SessionPool; introduced new interface for ConnectionManager; split tests of ConnectionPool and some cleanup in the classes
SNOW-902608
- new Connection Pool Manager version implementation
- unit tests for new pool; introduction of SessionFactory for unit testing of new pool manager and session pooling
- integration tests for two versions of Connection Pool
- ClearAllPools removes entire SessionPools collection in new pool
- Fixes to flaky tests of connection pool
@sfc-gh-mhofman sfc-gh-mhofman force-pushed the pool/SNOW-902608-new-pool-version branch from d309552 to c401f2c Compare November 9, 2023 13:08
@sfc-gh-mhofman sfc-gh-mhofman changed the base branch from master to pool/SNOW-860872-connection-pool November 9, 2023 13:09
@sfc-gh-mhofman sfc-gh-mhofman merged commit af36804 into pool/SNOW-860872-connection-pool Nov 9, 2023
19 of 20 checks passed
@sfc-gh-mhofman sfc-gh-mhofman deleted the pool/SNOW-902608-new-pool-version branch November 9, 2023 15:31
@github-actions github-actions bot locked and limited conversation to collaborators Nov 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants