Skip to content
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

Refs #21906 -- Changed dumpdata --database default to router.db_for_read. #12718

Closed
wants to merge 1 commit into from

Conversation

hramezani
Copy link
Member

@hramezani hramezani changed the title Refs #21906 -- Changed dumpdata --database default to router.db_for_write. Refs #21906 -- Changed dumpdata --database default to router.db_for_read. Apr 14, 2020
Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hramezani Thanks for this patch 👍 IMO we should change loaddata in the same commit, because changing only dumpdata will make this scenario even more error prone.

@@ -32,7 +32,7 @@ def add_arguments(self, parser):
)
parser.add_argument(
'--database',
default=DEFAULT_DB_ALIAS,
default=None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update help.

if not model._meta.proxy and router.allow_migrate_model(using, model):
_using = using
if _using is None:
_using = router.db_for_read(model)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would change

index df9eecc0f8..953f261ef4 100644
--- a/django/core/management/commands/dumpdata.py
+++ b/django/core/management/commands/dumpdata.py
@@ -67,7 +67,7 @@ class Command(BaseCommand):
     def handle(self, *app_labels, **options):
         format = options['format']
         indent = options['indent']
-        using = options['database']
+        database = options['database']
         excludes = options['exclude']
         output = options['output']
         show_traceback = options['traceback']

to simplify this.

We should also skip models with different db for read, instead of checking router.allow_migrate_model(), e.g.

using = router.db_for_read(model)
if not model._meta.proxy and (not database or using == database):
    if use_base_manager:
        ...

@@ -320,6 +320,8 @@ Management Commands
* The new :option:`dbshell -- ARGUMENTS <dbshell -->` option allows passing
extra arguments to the command-line client for the database.

* The default for :option:`dumpdata --database` changed to :meth:`db_for_read`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should go to the Backwards incompatible changes.

@hramezani
Copy link
Member Author

@felixxm Thanks for the review.

@hramezani Thanks for this patch IMO we should change loaddata in the same commit, because changing only dumpdata will make this scenario even more error prone.

I take a look at loaddata and I want to say it's hard to change it because:

  • In dumpdata we loop over models and fetch objects, so we can specify the database to read model objects from. but in loaddata we load data file by file and it each file we can have objects from multiple models.
  • In loaddata we create an atomic transaction on a database and load data to it. if we have a file contains object from several models, we should create multiple atomic transactions and it is hard to handle atomicity over multiple databases.
  • The deserialize depend on one DB, so we need to change them as well to fetch DB for each model from router

In general, I would say the loaddata implementation highly depends on just one database.

I am eager to finish it, so If you have any idea it would be appreciated.

Thanks

@felixxm
Copy link
Member

felixxm commented Apr 22, 2020

@hramezani IMO these changes cannot be implemented separately, because we will create an option to dump data from multiple databases but they will be useless for loaddata. It seems that we have two options:

  • we can raise an error in dumpdata for multiple "read" databases, or
  • we can try to handle this in loaddata with multiple transactions.

I will try to ask for opinions/ideas on the mailing list.

@felixxm
Copy link
Member

felixxm commented Apr 22, 2020

@hramezani "I will try to ask ..." -> "I would try to ask ..." 😄

@hramezani hramezani closed this Sep 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants