Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR switches from using multiprocessing.Manager to thread-local storage for strategy management in the e6data Python connector, and adds debug logging capabilities for strategy transitions.
- Removes multiprocessing.Manager initialization code and falls back to thread-local storage
- Adds comprehensive debug logging for strategy changes and query lifecycle events
- Introduces a debug mode flag that can be enabled per connection to track strategy transitions
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| setup.py | Version bump from 2.3.8 to 2.3.9 |
| e6data_python_connector/strategy.py | Removes multiprocessing.Manager initialization, simplifies to thread-local storage |
| e6data_python_connector/e6data_grpc.py | Adds debug logging system and immediately applies pending strategy changes |
| e6data_python_connector/dialect.py | Adds debug parameter support to connection URL parsing |
| README.md | Updates version badge to reflect new version |
| self._session_id = None | ||
| self.close() | ||
| self._create_client() | ||
| return self.get_session_id |
There was a problem hiding this comment.
Missing function call parentheses. This should be return self.get_session_id() to actually call the method, not return the method object.
| return self.get_session_id | |
| return self.get_session_id() |
| self.close() | ||
| self._create_client() | ||
| self._session_id = None | ||
| return self.get_session_id |
There was a problem hiding this comment.
Missing function call parentheses. This should be return self.get_session_id() to actually call the method, not return the method object.
| return self.get_session_id | |
| return self.get_session_id() |
|
|
||
| # Check for new strategy in prepare response | ||
| if hasattr(prepare_statement_response, 'new_strategy') and prepare_statement_response.new_strategy: | ||
| print(f"@@@@@@ New strategy: {prepare_statement_response.new_strategy}") |
There was a problem hiding this comment.
Debug print statement with temporary markers should be removed or replaced with proper logging using the _strategy_debug_log function.
| print(f"@@@@@@ New strategy: {prepare_statement_response.new_strategy}") | |
| _strategy_debug_log(f"New strategy detected: {prepare_statement_response.new_strategy}") |
| self.cluster_name = url.query.get("cluster-name") | ||
| self.secure = url.query.get("secure") == "true" | ||
| self.auto_resume = url.query.get("auto-resume", "true") == "true" # default to True | ||
| self.debug = url.query.get("debug", "false") == "true" # default to True |
There was a problem hiding this comment.
Comment incorrectly states 'default to True' when the code actually defaults to False. The comment should read '# default to False'.
| self.debug = url.query.get("debug", "false") == "true" # default to True | |
| self.debug = url.query.get("debug", "false") == "true" # default to False |
No description provided.