-
-
Notifications
You must be signed in to change notification settings - Fork 310
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
Data loss using DatabaseBackend when db connection unstable. #510
Comments
+1 This also just happened to us where randomly this defaulted the settings... Will try your fix thank you. We also had a "too many connections" issue on our DB just before this happened. This has caused quite a bit of havoc here :) |
Hi django-constance team, I also had ~3 instances of this happening in the past month. We're using a Redis backend and the feature flags got wiped. At first I suspected our Redis instance got flushed or something like that, but we couldn't find any evidence to support that. Thanks! |
Would this PR address the issue? https://github.com/jazzband/django-constance/pull/444/files This issue also looks related: #348 |
Feel free to add a test case so we can merge this possible solution. |
No, it won't solve issue since this check for Even my solution is not ideal since it will expose database errors to users of this package. IMHO: Ideal solution is to add new exception like ConfigBackendError that will be risen instead of (OperationalError, ProgrammingError or redis client similar errors) so Config class can handle cases when used config backend is not working for some reason |
when trying to get a value while there is no database connection, the OperationalError will be raised. We should allow it to be raised instead of suppressing it. Related to jazzband#510
Re-reading the code and tried out a small unit test and I agree with you, this doesn't quite solve the issue.
It makes sense to me that the errors would be surfaced; users of django-constance are also using Django which is what raises the errors. If there are other database/redis errors being raised during a request processing or in a background worker, the error types would point to the same database issue. Raising a new type of error could potentially obfuscate what's happening. I created a draft PR: https://github.com/jazzband/django-constance/pull/525/files |
Redis backend is not part of the Django
It won't because I suggest to wrap backend related error in ConfigBackendError, it's common practice even Django is doing the same thing with it's database backends wrapping all possible driver errors in Django standardised errors like OperationalError Because if django-constance provides the same API for different config backends it also should provide the same exceptions that could be catched with predefined exceptions. Example: from constance import config
from constance.exceptions import ConfigBackendError
try:
config.FOO_CONSTANCE
except ConfigBackendError:
# handle case when config is not working And I as a developer don't need to change my code implementation just because I switched backend from database to Redis |
I am able to reproduce this locally:
A test for this already exists: https://github.com/jazzband/django-constance/pull/525/files#diff-81b4098763925e2da30d4f500777337c3f06a96f6eefa70802858ba8d7ff3909R45 |
I've taken an approach that has the same effect, but also makes the code easier to understand. Please see #538 This does away with exception handling in that block altogether. The only reason the handling was there was because the code pre-dated the introduction of .first(), which handles the case of no results without throwing a DoesNotExist error. If the DB is down, the database backend will now behave the same way as every other backend (redis, memory) -- which is to say (as @sebastianmanger put it): 💥 I'll let this comment and PR sit here for a bit for comments, and merge it next week. |
Describe the problem
I noticed that once a month or two some most used constance would just reset to default values.
After some insvestigation I found out that it was caused when db connection was unstable (db was overloaded or restarted for example)
Steps to reproduce
I don't think it would be easy to reproduce because of percise timings but it can be reproduced with some mocks in tests.
In method get of DatabaseBackend https://github.com/jazzband/django-constance/blob/master/constance/backends/database/__init__.py#L69
As you can see when we try to
get
value from database we just ignore exceptions OperationalError and ProgrammingError. Let's assume db was overloaded and caused OperationalError for example so when we return value it is None.Now we go to wrapper class Config https://github.com/jazzband/django-constance/blob/master/constance/base.py#L20
Just like I said before method
get
returns None and in this case wrapperConfig.__getattr__
just sets value too default and at this time all database issues are resolved and that is how we just lost data of a constance.How I fixed issue right now:
I just inherited DatabaseBackend and removed OperationalError and ProgrammingError from
except
blockSystem configuration
The text was updated successfully, but these errors were encountered: