There was a problem hiding this comment.
This PR implements HTTP connection pooling for the remote online store client to improve performance by reusing TCP/TLS connections across requests. The changes introduce a thread-safe session manager, configurable connection pool settings, and automatic idle timeout management.
Changes:
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file| sdk/python/feast/permissions/client/http_auth_requests_wrapper.py | Implements HttpSessionManager with connection pooling, retry logic, and idle timeout |
| sdk/python/feast/rest_error_handler.py | Updates decorator to use cached sessions and extract connection pool config |
| sdk/python/feast/infra/online_stores/remote.py | Adds connection pool config fields and close() method |
| sdk/python/tests/unit/permissions/auth/client/test_http_session_manager.py | Adds comprehensive test suite for session manager |
| sdk/python/tests/unit/test_rest_error_decorator.py | Updates tests to close cached sessions before running |
| docs/reference/online-stores/remote.md | Documents connection pooling configuration and usage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
| {"Authorization": f"Bearer {auth_token}"} | ||
| ) | ||
| except Exception as e: | ||
| logger.warning(f"Failed to refresh auth token: {e}") |
There was a problem hiding this comment.
The error message 'Failed to refresh auth token' is logged but the exception is silently swallowed. This could lead to authentication failures being unnoticed. Consider either propagating the exception or providing more context about the consequences of continuing with a potentially stale token.
| logger.warning(f"Failed to refresh auth token: {e}") | |
| logger.warning(f"Failed to refresh auth token: {e}") | |
| raise |
Sorry, something went wrong.
What this PR does / why we need it:
Misc
New Features
Configuration Options