#23718 closed Bug (fixed)
TEST_MIRROR setting doesn't work as expected (and has no tests)
Reported by: | Ilya Baryshev | Owned by: | Sarah Boyce |
---|---|---|---|
Component: | Documentation | Version: | 1.7 |
Severity: | Normal | Keywords: | replica testing |
Cc: | Carlton Gibson, Patrick Cloke | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
TEST_MIRROR promises "connection to slave will be redirected to point at default. As a result, writes to default will appear on slave"
I've set up a minimum django project (using postgres backed) to demonstrate behavior.
def test_fixture(self): MyModel.objects.using('default').create(name=1) MyModel.objects.using('slave').create(name=2) MyModel.objects.using('slave').create(name=3) self.assertEqual(list(map(repr, MyModel.objects.using('default').all())), list(map(repr, MyModel.objects.using('slave').all())))
Both lists should be equal, because replica queries should be hitting default instead.
It appears not to be the case for Django>=1.4 up to latest 1.7.1 (but actually passes against 1.3.7)
AssertionError: Lists differ: ['<MyModel: 1>', '<MyModel: 2>... != ['<MyModel: 2>', '<MyModel: 3>... First differing element 0: <MyModel: 1> <MyModel: 2> First list contains 1 additional elements. First extra element 2: <MyModel: 3> - ['<MyModel: 1>', '<MyModel: 2>', '<MyModel: 3>'] ? ^ ---------------- + ['<MyModel: 2>', '<MyModel: 3>'] ? ^
Here is a project I used to test: https://github.com/coagulant/test_mirror
Change History (35)
comment:1 by , 10 years ago
comment:2 by , 10 years ago
I've run git bisect, and first commit with regression happened to be: 905e33f84a1a10e4f0183d879c52076ef876fc3b, which is related to #16885
Here is a log of my bisect: https://gist.github.com/coagulant/d04b3e729b851784a93a
comment:3 by , 10 years ago
Summary: | TEST_MIRROR setting doesn't work as expected → TEST_MIRROR setting doesn't work as expected (and has no tests) |
---|---|
Triage Stage: | Unreviewed → Accepted |
I haven't verified the issue, but I noticed there are no tests for TEST['MIRROR']
(renamed on master) so we should at least add some. Also the test in the "regression commit" seems to be gone -- not sure if its removal was intentional or not.
comment:6 by , 10 years ago
The behavior in the bug description is sort-of expected, because of transactions (default
and slave
are the same database -- in the test project, they are defined this way even regardless of TEST_MIRROR
-- but accessed through different connections, and so, separate transactions).
I'm saying sort-of expected because it seems the test is running in a transaction on default
but not on slave
, which is a little surprising -- but not the problem claimed.
I don't think we want to force test-cases to run each test in transactions on all databases -- I'm not sure that even makes sense; but we should probably document that tests using more than one database should be TransactionTestCase
s.
comment:7 by , 10 years ago
Is it possible to have the mirrors share a connection? Using TransactionTestCase
has the additional disadvantage of preventing multiprocessed test runs, which works fine when each test runs in a transaction.
comment:8 by , 9 years ago
I just ran into this as well. Based on the documentation, I expected to set
DATABASES = { 'default': { ... }, 'replica': { ... 'TEST_MIRROR': 'default', # 'TEST': {'MIRROR': 'default'}, # I tried this too. } }
and have my tests read from replica and write to default, which should be the same database and act as if I had just one database defined as default
.
comment:9 by , 9 years ago
I just rediscovered that test mirrors require TransactionTestCase
.
I'm wondering if it would be possible to share the database connection instead of having two connections with the same parameters.
If that doesn't work, we can document this limitation instead.
comment:10 by , 9 years ago
Just ran into this, but with raw queries instead. Using the connection directly does not work even with TransactionTestCase
.
So for now I'm using this hacky workaround:
from django.db import connections class ReplicationTestCase(TestCase): """ Redirect raw queries to the replica, ie. from django.db import connections cursor = connections['replica'].cursor() cursor.execute(...) # Is run directly on master in tests """ def setUp(self): super(ReplicationTestCase, self).setUp() connections['replica']._orig_cursor = connections['replica'].cursor connections['replica'].cursor = connections['master'].cursor def tearDown(self): connections['replica'].cursor = connections['replica']._orig_cursor super(ReplicationTestCase, self).tearDown()
As an added benefit it works for model managers too even though I'm not using TransactionTestCase
indicating that the connection might be able to be shared in a similar way?
comment:11 by , 8 years ago
Is this ever going to be fixed? Setting the mirror should just copy the cursor so no matter where the slave DB is called it redirects to the master.
comment:12 by , 8 years ago
It will be fixed when someone submits a patch and another person reviews it.
comment:13 by , 7 years ago
I have created a small patch, the PR is here https://github.com/django/django/pull/9603
The solution I went for is that when the MIRROR
is specified then the mirror uses the same connection as the mirrored one.
If the tests look a bit redundant please let me know.
comment:14 by , 7 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:15 by , 7 years ago
Has patch: | set |
---|
comment:18 by , 7 years ago
Patch needs improvement: | unset |
---|
comment:19 by , 7 years ago
Question from PR:
The PR currently uses mocks in the tests, but it's much simpler (for the current test and presumably into the future) and much clearer to just declare an extra replica
alias with the TEST["MIRROR"]
setting.
This second approach would require adjustments to test settings for Jenkins, the Django-Box VM and all devs' settings running against other DBs.
So the question is can we make that change?
comment:20 by , 7 years ago
Cc: | added |
---|
comment:21 by , 7 years ago
Patch needs improvement: | set |
---|
comment:22 by , 5 years ago
Owner: | changed from | to
---|
Planning to work on this since affecting our use of testing using TEST_MIRROR
and TestCase
.
I think the right approach here is to have mirror connections swapped to the connection object they mirror but only when using TestCase to allow proper transaction testing to take place. That's something the previous patch wasn't doing appropriately.
comment:23 by , 5 years ago
Hi Team,
We also noticed an issue here which happens when you have a default + read_replica config, and you have a router that only selects a read_replica when considering particular models. If you have those conditions, _and_ a data migration that uses both Job and User, you will have issues because the "default" connection config will be pointing to your test_myschema
, whereas the read_replica
config will still be pointing to myschema
.
e.g.,
models.py: Job: user_id := FK(User) DBRouter: db_for_write := 'read_replica' if model == Job else 'default' settings: DATABASES = { 'default': { ... }, 'read_replica': { # <configs>... 'TEST': {'MIRROR': 'default'} } } 0001_migration.py: # ... for job in Job.objects.all(): # using apps.get_model() job.user.id # will error out with __fake__.DoesNotExist
In the case above, if your "live" read_replica schema has any job data, it will try to run that last line, but there won't be any Users in the test_schema
to match to.
Not sure if we're doing something wrong, but just letting you know that there are cases where a custom DBrouter could route to a config that should be a test mirror, but isn't.
Using python 3.6, Django 1.11.20, MySQL 5.7
comment:24 by , 5 years ago
@Simon Charette If you haven't started working on this, I would like to give this a try.
comment:25 by , 5 years ago
@Rag Sagar.V, sure feel free to assign the ticket to yourself.
I'd start by having a look at the previous PR and make sure to understand the rationale behind why it got rejected.
For what it's worth I gave this ticket a 2-3 hours try by trying to point connections['mirror']
at connections['mirrored']
Python objects for the duration of TestCase
and I got some failures related to transaction handling. I think Tim did a good job at nailing the issues with this approach. In short transaction handing is alias based so if something goes wrong with connections['mirrored']
then it's also surfaced in connections['mirror']
which differs from what would happen in a real life scenario.
The further I got from getting things working were by setting the isolation level on the miror/replica to READ UNCOMMITED
for the duration of the test to allow it to see changes within the testcase transaction. This isolation level is not supported on all backends though (e.g. PostgreSQL) and was quite unreliable on MySQL from my minimal testing.
That one might be hard one if you're not familiar with how TestCase
transactions wrapping takes place but you'll definitely learn a lot by giving this ticket a shot and seeing how the suite fails. Happy to review your changes if you get stuck on Github.
comment:26 by , 5 years ago
Owner: | changed from | to
---|
comment:27 by , 5 years ago
@Simon Charette Since READ_UNCOMMITED
is not supported in postgres, Should I explore further in that direction? The solution that comes up on my mind is pointing mirror connections to mirrored for the duration of the TestCase
. But that has the issues you pointed out. So I am bit confused which way I should go.
comment:28 by , 5 years ago
Rag Sagar, I'm afraid none of the above solution will work appropriately.
Here's the problem:
If you configure mirrors to create a new connection to mirrored connection settings then changes performed within a transaction using the mirrored connection won't be visible in the mirror connection per READ COMMITED
transaction isolation definition. That's the case right now.
Using READ UNCOMMITTED
on the mirror connection seems to be a no-go from limited support and the fact MySQL seems inconsistent and reusing the same connection has confuses transaction management.
The only solutions I see going forward are:
- Document this caveats of
TEST_MIRROR
andTestCase
transaction wrapping interactions. - Invest more time trying to get the
connections
repointing duringTestCase.setUpClass
working to get the suite passing and document the new caveats.
In both cases we could at least do 1. or warn about it to make it clear this is not currently supported.
comment:29 by , 5 years ago
Cc: | added |
---|
comment:31 by , 3 years ago
Component: | Testing framework → Documentation |
---|---|
Owner: | changed from | to
Patch needs improvement: | unset |
comment:32 by , 3 years ago
Patch needs improvement: | set |
---|
comment:35 by , 23 months ago
Triage Stage: | Accepted → Ready for checkin |
---|
It would be quite helpful if you could convert your test to one for Django's test suite and then bisect to determine the commit in Django that introduced the regression.