SNOW-1720835 param protect thread-safe client side changes #2401
Conversation
…iables-thread-safe
…iables-thread-safe
…iables-thread-safe
…iables-thread-safe
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
sfc-gh-aalam
changed the base branch from
main
to
aalam-SNOW-1418523-make-internal-session-variables-thread-safe
src/snowflake/snowpark/session.py
Outdated
| ) | ||
| ) | ||
| self._thread_store = ( | ||
| threading.local() if self._thread_safe_session_enabled else dict() |
There was a problem hiding this comment.
instead of dict, maybe just use optional here, and only use the _thread_store value if the parameter is on
There was a problem hiding this comment.
I would then have to create something like __analyzer and __conn since the _analyzer is already created as a property now. I think since we have tests passing, it should be okay. I am using a DummyThreadLocal class now. Everything will be removed once I rollout parameter.
sfc-gh-aalam
changed the title
…ad-safe' into SNOW-1720835-param-protect-client-side-changes
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
| @@ -32,6 +35,21 @@ | |||
| from tests.utils import IS_IN_STORED_PROC, IS_LINUX, IS_WINDOWS, TestFiles, Utils | |||
|
|
|||
|
|
|||
| @pytest.fixture(scope="module") | |||
There was a problem hiding this comment.
let's use a function scope here to avoid messing with other tests
There was a problem hiding this comment.
renamed to threadsafe_session. this would not mess with others, right?
tests/integ/test_multithreading.py
Outdated
| @@ -32,6 +35,21 @@ | |||
| from tests.utils import IS_IN_STORED_PROC, IS_LINUX, IS_WINDOWS, TestFiles, Utils | |||
|
|
|||
|
|
|||
| @pytest.fixture(scope="module") | |||
| def session( | |||
There was a problem hiding this comment.
let's call this mutlithread_session instead of session
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
…ub.com:snowflakedb/snowpark-python into SNOW-1720835-param-protect-client-side-changes
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
sfc-gh-aalam
deleted the
SNOW-1720835-param-protect-client-side-changes
branch
Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes SNOW-1720835
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
This PR adds param protection and uses
DummyLockandDummyThreadLocalobjects when thread-safe session is not enabled for session. This will be removed once thread-safe session is fully rolled out.