SNOW-937188 pool mode where changed session gets destroyed [WIP] #934
Conversation
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
sfc-gh-mhofman
changed the base branch from
master
to
pool/SNOW-860872-connection-pool
…some of its settings gets changed and no longer match the pool initial setup
sfc-gh-mhofman
force-pushed
the
pool/SNOW-937188-changed-session
branch
from
c44f234 to
e727d1a
Compare
| if (session.SessionPropertiesChanged && _poolConfig.ChangedSession == ChangedSessionBehavior.Destroy) | ||
| { | ||
| session.SetPooling(false); | ||
| Task.Run(() => session.CloseAsync(CancellationToken.None)).ConfigureAwait(false); |
There was a problem hiding this comment.
why just not to forget about the session letting dispose by Garbage Collector do the job?
what is the difference between this case and a case when someone manually set poolingEnabled = false on the session? In case of session.poolingEnabled = false we just decrease busy session counter and return false. I think both cases are very similar so they should be handled in a similar way.
| internal string schema; | ||
| internal string role; |
There was a problem hiding this comment.
should user be also here?
There was a problem hiding this comment.
there's no such thing in login response's SessionData object:
| { | ||
| this.schema = schemaName; | ||
| if (!String.IsNullOrEmpty(initialSessionValue) && initialSessionValue != finalSessionValue) |
There was a problem hiding this comment.
shouldn't we use for non-quoted values use ignoring case comparison?
| { | ||
| [TestFixture] | ||
| [NonParallelizable] | ||
| public class ConnectionChangedSessionIT : SFBaseTest |
There was a problem hiding this comment.
maybe ConnectionMultiplePoolsChangedSessionIT? it would be more visible that it is a test for the new pool
| [NonParallelizable] | ||
| public class ConnectionChangedSessionIT : SFBaseTest | ||
| { | ||
| private readonly QueryExecResponseData _queryExecResponseChangedRole = new() |
There was a problem hiding this comment.
BTW do we want it to work for old pool as well? As I see the implementation - it would work for old pool as well. Don't we want to cover that in tests?
There was a problem hiding this comment.
lets have it only in latest pool (the least changes to prev the better)
| [NonParallelizable] | ||
| public class ConnectionChangedSessionIT : SFBaseTest | ||
| { | ||
| private readonly QueryExecResponseData _queryExecResponseChangedRole = new() |
There was a problem hiding this comment.
I think all of the parameters that can cause not returning session to the pool should be covered in tests (so making one of them excluded from list should make some test to fail). But currently I think the situation is different. For example if the role has changed - it is only checked in the test for default pool mode which is OriginalPool, and is not covered in Destroy test. So I would add more unit tests to SFSessionTests.
There was a problem hiding this comment.
Agree but on the other side any changed is handled equally
https://github.com/snowflakedb/snowflake-connector-net/pull/934/files#diff-c36410e4608c5d3a815bdfa7327569a75af2ead26f4e5e67ab0f5f421120f6bdR476
I'm not sure we want to run each test 16 times in this case?
sfc-gh-mhofman
changed the title
Description
When ChangedSession=Destroy is set in a connection string and a session is affected with a USE DATABASE/SCHEMA/ROLE/WAREHOUSE it is treated as no-longer matching original pool and gets destroyed on closing.
Its a non default pool behavior.
Checklist
dotnet test)