-
Notifications
You must be signed in to change notification settings - Fork 429
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
feat: Add support for replicas and cross region replicas #3300
Merged
Merged
Changes from 5 commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
637092f
Add support for replicas and cross region replicas
zachaysan 65ba9d6
Set TTL to proper timeout
zachaysan 9977571
Create tests for the replica router logic
zachaysan 59329b4
Standardize wording
zachaysan 088e32e
Trigger Build
zachaysan 7e8b205
Use replica read strategy enum
zachaysan dd76946
Switch to env enum
zachaysan 0f9f357
Add enum and custom error
zachaysan ce75308
Remove return value
zachaysan c941310
Merge branch 'main' into feat/add_replica_support
zachaysan 2953dcf
Add replication docs
zachaysan 6c8e216
Remove former replication docs
zachaysan 345cc7b
Merge branch 'main' into feat/add_replica_support
zachaysan 266935a
Trigger build
zachaysan e0b1414
Add db fixture
zachaysan e4df146
Merge branch 'main' into feat/add_replica_support
zachaysan 644e4c7
Trigger build
zachaysan 7b8aa42
Trigger build
zachaysan 1f9ab5e
Move exception and add newline
zachaysan 3c294fa
Merge branch 'main' into feat/add_replica_support
zachaysan 01a2a70
Add cross region
zachaysan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,188 @@ | ||
from pytest_django.fixtures import SettingsWrapper | ||
from pytest_mock import MockerFixture | ||
|
||
from app.routers import PrimaryReplicaRouter, connection_check | ||
from users.models import FFAdminUser | ||
|
||
|
||
def test_connection_check_to_default_database(db: None, reset_cache: None) -> None: | ||
# When | ||
connection_check_works = connection_check("default") | ||
|
||
# Then | ||
assert connection_check_works is True | ||
|
||
|
||
def test_replica_router_db_for_read_with_one_offline_replica( | ||
settings: SettingsWrapper, | ||
mocker: MockerFixture, | ||
reset_cache: None, | ||
) -> None: | ||
# Given | ||
settings.NUM_DB_REPLICAS = 4 | ||
|
||
# Set unused cross regional db for testing non-inclusion. | ||
settings.NUM_CROSS_REGION_DB_REPLICAS = 2 | ||
settings.REPLICA_READ_STRATEGY = "DISTRIBUTED" | ||
|
||
conn_patch = mocker.MagicMock() | ||
conn_patch.is_usable.side_effect = (False, True) | ||
create_connection_patch = mocker.patch( | ||
"app.routers.connections.create_connection", return_value=conn_patch | ||
) | ||
|
||
router = PrimaryReplicaRouter() | ||
|
||
# When | ||
result = router.db_for_read(FFAdminUser) | ||
|
||
# Then | ||
# Read strategy DISTRIBUTED is random, so just this is a check | ||
# against loading the primary or one of the cross region replicas | ||
assert result.startswith("replica_") | ||
|
||
# Check that the number of replica call counts is as expected. | ||
conn_call_count = 2 | ||
assert create_connection_patch.call_count == conn_call_count | ||
assert conn_patch.is_usable.call_count == conn_call_count | ||
|
||
|
||
def test_replica_router_db_for_read_with_local_offline_replicas( | ||
settings: SettingsWrapper, | ||
mocker: MockerFixture, | ||
reset_cache: None, | ||
) -> None: | ||
# Given | ||
settings.NUM_DB_REPLICAS = 4 | ||
|
||
# Use cross regional db for fallback after replicas. | ||
settings.NUM_CROSS_REGION_DB_REPLICAS = 2 | ||
settings.REPLICA_READ_STRATEGY = "DISTRIBUTED" | ||
|
||
conn_patch = mocker.MagicMock() | ||
|
||
# All four replicas go offline and so does one of the cross | ||
# regional replica as well, before finally the last cross | ||
# region replica is finally connected to. | ||
conn_patch.is_usable.side_effect = ( | ||
False, | ||
False, | ||
False, | ||
False, | ||
False, | ||
True, | ||
) | ||
create_connection_patch = mocker.patch( | ||
"app.routers.connections.create_connection", return_value=conn_patch | ||
) | ||
|
||
router = PrimaryReplicaRouter() | ||
|
||
# When | ||
result = router.db_for_read(FFAdminUser) | ||
|
||
# Then | ||
# Read strategy DISTRIBUTED is random, so just this is a check | ||
# against loading the primary or one of the cross region replicas | ||
assert result.startswith("cross_region_replica_") | ||
|
||
# Check that the number of replica call counts is as expected. | ||
conn_call_count = 6 | ||
assert create_connection_patch.call_count == conn_call_count | ||
assert conn_patch.is_usable.call_count == conn_call_count | ||
|
||
|
||
def test_replica_router_db_for_read_with_all_offline_replicas( | ||
settings: SettingsWrapper, | ||
mocker: MockerFixture, | ||
reset_cache: None, | ||
) -> None: | ||
# Given | ||
settings.NUM_DB_REPLICAS = 4 | ||
settings.NUM_CROSS_REGION_DB_REPLICAS = 2 | ||
settings.REPLICA_READ_STRATEGY = "DISTRIBUTED" | ||
|
||
conn_patch = mocker.MagicMock() | ||
|
||
# All replicas go offline. | ||
conn_patch.is_usable.return_value = False | ||
create_connection_patch = mocker.patch( | ||
"app.routers.connections.create_connection", return_value=conn_patch | ||
) | ||
|
||
router = PrimaryReplicaRouter() | ||
|
||
# When | ||
result = router.db_for_read(FFAdminUser) | ||
|
||
# Then | ||
# Fallback to primary database if all replicas are offline. | ||
assert result == "default" | ||
|
||
# Check that the number of replica call counts is as expected. | ||
conn_call_count = 6 | ||
assert create_connection_patch.call_count == conn_call_count | ||
assert conn_patch.is_usable.call_count == conn_call_count | ||
|
||
|
||
def test_replica_router_db_with_sequential_read( | ||
settings: SettingsWrapper, | ||
mocker: MockerFixture, | ||
reset_cache: None, | ||
) -> None: | ||
# Given | ||
settings.NUM_DB_REPLICAS = 100 | ||
settings.NUM_CROSS_REGION_DB_REPLICAS = 2 | ||
settings.REPLICA_READ_STRATEGY = "SEQUENTIAL" | ||
|
||
conn_patch = mocker.MagicMock() | ||
|
||
# First replica is offline, so must fall back to second one. | ||
conn_patch.is_usable.side_effect = (False, True) | ||
create_connection_patch = mocker.patch( | ||
"app.routers.connections.create_connection", return_value=conn_patch | ||
) | ||
|
||
router = PrimaryReplicaRouter() | ||
|
||
# When | ||
result = router.db_for_read(FFAdminUser) | ||
|
||
# Then | ||
# Fallback from first replica to second one. | ||
assert result == "replica_2" | ||
|
||
# Check that the number of replica call counts is as expected. | ||
conn_call_count = 2 | ||
assert create_connection_patch.call_count == conn_call_count | ||
assert conn_patch.is_usable.call_count == conn_call_count | ||
|
||
|
||
def test_replica_router_db_no_replicas( | ||
settings: SettingsWrapper, | ||
mocker: MockerFixture, | ||
reset_cache: None, | ||
) -> None: | ||
# Given | ||
settings.NUM_DB_REPLICAS = 0 | ||
settings.NUM_CROSS_REGION_DB_REPLICAS = 0 | ||
|
||
conn_patch = mocker.MagicMock() | ||
|
||
# All replicas should be ignored. | ||
conn_patch.is_usable.return_value = True | ||
create_connection_patch = mocker.patch( | ||
"app.routers.connections.create_connection", return_value=conn_patch | ||
) | ||
zachaysan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
router = PrimaryReplicaRouter() | ||
|
||
# When | ||
result = router.db_for_read(FFAdminUser) | ||
|
||
# Then | ||
# Should always use primary database. | ||
assert result == "default" | ||
conn_call_count = 0 | ||
assert create_connection_patch.call_count == conn_call_count | ||
assert conn_patch.is_usable.call_count == conn_call_count |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
nit: imo these should be separated by a (blank) new line whitespace.
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.
I think it looks fine my way, but I don't mind doing it your way either so I've updated it.