-
Notifications
You must be signed in to change notification settings - Fork 73
Fix/issue 465 async sentinel connection pool #522
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,3 +1,19 @@ | ||||||||||||||||||||
| """Tests for Redis Sentinel URL connection handling. | ||||||||||||||||||||
|
|
||||||||||||||||||||
| This module tests the RedisConnectionFactory's ability to create Redis clients | ||||||||||||||||||||
| from Sentinel URLs (redis+sentinel://...). It verifies: | ||||||||||||||||||||
|
|
||||||||||||||||||||
| 1. Correct Sentinel class selection (AsyncSentinel for async, Sentinel for sync) | ||||||||||||||||||||
| 2. URL parsing (hosts, ports, service name, database, authentication) | ||||||||||||||||||||
| 3. Proper kwargs passthrough to Sentinel and master_for() | ||||||||||||||||||||
| 4. Error handling for connection failures | ||||||||||||||||||||
|
|
||||||||||||||||||||
| These tests use mocking to avoid requiring a real Sentinel deployment. | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Related: GitHub Issue #465 - Async Sentinel connections were incorrectly using | ||||||||||||||||||||
| the sync SentinelConnectionPool, causing runtime failures. | ||||||||||||||||||||
| """ | ||||||||||||||||||||
|
|
||||||||||||||||||||
| from unittest.mock import MagicMock, patch | ||||||||||||||||||||
|
|
||||||||||||||||||||
| import pytest | ||||||||||||||||||||
|
|
@@ -12,7 +28,14 @@ def test_sentinel_url_connection(use_async): | |||||||||||||||||||
| "redis+sentinel://username:password@host1:26379,host2:26380/mymaster/0" | ||||||||||||||||||||
| ) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| with patch("redisvl.redis.connection.Sentinel") as mock_sentinel: | ||||||||||||||||||||
| # Use appropriate Sentinel class based on sync/async | ||||||||||||||||||||
| sentinel_patch_target = ( | ||||||||||||||||||||
| "redisvl.redis.connection.AsyncSentinel" | ||||||||||||||||||||
| if use_async | ||||||||||||||||||||
| else "redisvl.redis.connection.Sentinel" | ||||||||||||||||||||
| ) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| with patch(sentinel_patch_target) as mock_sentinel: | ||||||||||||||||||||
| mock_master = MagicMock() | ||||||||||||||||||||
| mock_sentinel.return_value.master_for.return_value = mock_master | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
@@ -42,7 +65,14 @@ def test_sentinel_url_connection(use_async): | |||||||||||||||||||
| def test_sentinel_url_connection_no_auth_no_db(use_async): | ||||||||||||||||||||
| sentinel_url = "redis+sentinel://host1:26379,host2:26380/mymaster" | ||||||||||||||||||||
|
|
||||||||||||||||||||
| with patch("redisvl.redis.connection.Sentinel") as mock_sentinel: | ||||||||||||||||||||
| # Use appropriate Sentinel class based on sync/async | ||||||||||||||||||||
| sentinel_patch_target = ( | ||||||||||||||||||||
| "redisvl.redis.connection.AsyncSentinel" | ||||||||||||||||||||
| if use_async | ||||||||||||||||||||
| else "redisvl.redis.connection.Sentinel" | ||||||||||||||||||||
| ) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| with patch(sentinel_patch_target) as mock_sentinel: | ||||||||||||||||||||
| mock_master = MagicMock() | ||||||||||||||||||||
| mock_sentinel.return_value.master_for.return_value = mock_master | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
@@ -72,7 +102,14 @@ def test_sentinel_url_connection_no_auth_no_db(use_async): | |||||||||||||||||||
| def test_sentinel_url_connection_error(use_async): | ||||||||||||||||||||
| sentinel_url = "redis+sentinel://host1:26379,host2:26380/mymaster" | ||||||||||||||||||||
|
|
||||||||||||||||||||
| with patch("redisvl.redis.connection.Sentinel") as mock_sentinel: | ||||||||||||||||||||
| # Use appropriate Sentinel class based on sync/async | ||||||||||||||||||||
| sentinel_patch_target = ( | ||||||||||||||||||||
| "redisvl.redis.connection.AsyncSentinel" | ||||||||||||||||||||
| if use_async | ||||||||||||||||||||
| else "redisvl.redis.connection.Sentinel" | ||||||||||||||||||||
| ) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| with patch(sentinel_patch_target) as mock_sentinel: | ||||||||||||||||||||
| mock_sentinel.return_value.master_for.side_effect = ConnectionError( | ||||||||||||||||||||
| "Test connection error" | ||||||||||||||||||||
| ) | ||||||||||||||||||||
|
|
@@ -85,3 +122,137 @@ def test_sentinel_url_connection_error(use_async): | |||||||||||||||||||
| RedisConnectionFactory.get_redis_connection(sentinel_url) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| mock_sentinel.assert_called_once() | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| def test_async_sentinel_uses_async_sentinel_class(): | ||||||||||||||||||||
| """Test that async connections use AsyncSentinel (fix for issue #465).""" | ||||||||||||||||||||
| sentinel_url = "redis+sentinel://host1:26379/mymaster" | ||||||||||||||||||||
|
|
||||||||||||||||||||
| # Track which Sentinel class is called | ||||||||||||||||||||
| sync_sentinel_called = False | ||||||||||||||||||||
| async_sentinel_called = False | ||||||||||||||||||||
|
|
||||||||||||||||||||
| def track_sync_sentinel(*args, **kwargs): | ||||||||||||||||||||
| nonlocal sync_sentinel_called | ||||||||||||||||||||
| sync_sentinel_called = True | ||||||||||||||||||||
| mock = MagicMock() | ||||||||||||||||||||
| mock.master_for.return_value = MagicMock() | ||||||||||||||||||||
| return mock | ||||||||||||||||||||
|
|
||||||||||||||||||||
| def track_async_sentinel(*args, **kwargs): | ||||||||||||||||||||
| nonlocal async_sentinel_called | ||||||||||||||||||||
| async_sentinel_called = True | ||||||||||||||||||||
| mock = MagicMock() | ||||||||||||||||||||
| mock.master_for.return_value = MagicMock() | ||||||||||||||||||||
| return mock | ||||||||||||||||||||
|
|
||||||||||||||||||||
| with ( | ||||||||||||||||||||
| patch("redisvl.redis.connection.Sentinel", side_effect=track_sync_sentinel), | ||||||||||||||||||||
| patch( | ||||||||||||||||||||
| "redisvl.redis.connection.AsyncSentinel", side_effect=track_async_sentinel | ||||||||||||||||||||
| ), | ||||||||||||||||||||
| ): | ||||||||||||||||||||
|
Comment on lines
+149
to
+154
|
||||||||||||||||||||
| with pytest.warns(DeprecationWarning): | ||||||||||||||||||||
| RedisConnectionFactory.get_async_redis_connection(sentinel_url) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| # Verify AsyncSentinel was called, not sync Sentinel | ||||||||||||||||||||
| assert async_sentinel_called, "AsyncSentinel should be called for async connections" | ||||||||||||||||||||
| assert ( | ||||||||||||||||||||
| not sync_sentinel_called | ||||||||||||||||||||
| ), "Sync Sentinel should NOT be called for async connections" | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| def test_sync_sentinel_uses_sync_sentinel_class(): | ||||||||||||||||||||
| """Test that sync connections use sync Sentinel.""" | ||||||||||||||||||||
| sentinel_url = "redis+sentinel://host1:26379/mymaster" | ||||||||||||||||||||
|
|
||||||||||||||||||||
| # Track which Sentinel class is called | ||||||||||||||||||||
| sync_sentinel_called = False | ||||||||||||||||||||
| async_sentinel_called = False | ||||||||||||||||||||
|
|
||||||||||||||||||||
| def track_sync_sentinel(*args, **kwargs): | ||||||||||||||||||||
| nonlocal sync_sentinel_called | ||||||||||||||||||||
| sync_sentinel_called = True | ||||||||||||||||||||
| mock = MagicMock() | ||||||||||||||||||||
| mock.master_for.return_value = MagicMock() | ||||||||||||||||||||
| return mock | ||||||||||||||||||||
|
|
||||||||||||||||||||
| def track_async_sentinel(*args, **kwargs): | ||||||||||||||||||||
| nonlocal async_sentinel_called | ||||||||||||||||||||
| async_sentinel_called = True | ||||||||||||||||||||
| mock = MagicMock() | ||||||||||||||||||||
| mock.master_for.return_value = MagicMock() | ||||||||||||||||||||
| return mock | ||||||||||||||||||||
|
|
||||||||||||||||||||
| with ( | ||||||||||||||||||||
| patch("redisvl.redis.connection.Sentinel", side_effect=track_sync_sentinel), | ||||||||||||||||||||
| patch( | ||||||||||||||||||||
| "redisvl.redis.connection.AsyncSentinel", side_effect=track_async_sentinel | ||||||||||||||||||||
| ), | ||||||||||||||||||||
|
Comment on lines
+187
to
+191
|
||||||||||||||||||||
| with ( | |
| patch("redisvl.redis.connection.Sentinel", side_effect=track_sync_sentinel), | |
| patch( | |
| "redisvl.redis.connection.AsyncSentinel", side_effect=track_async_sentinel | |
| ), | |
| with patch( | |
| "redisvl.redis.connection.Sentinel", side_effect=track_sync_sentinel | |
| ), patch( | |
| "redisvl.redis.connection.AsyncSentinel", side_effect=track_async_sentinel |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstring describes the URL format as requiring
/service, but_parse_sentinel_url()(and the tests in this PR) allow the service name to be omitted and default to"mymaster". Please update the format description here to reflect that the service segment is optional (or explicitly document the default).