feat: give users the option to run the json migration asyncly #495

aqeelat · 2023-01-12T14:47:42Z

No description provided.

hramezani · 2023-01-16T10:58:41Z

Thanks @aqeelat for your effort!

I assume that users who want to migrate normally, just run migrate command and see a changes_text in the model and everything else is like before. right?

The general approach is ok for me we need to document it.

@alieh-rymasheuski do you have any idea here?

hramezani · 2023-01-16T11:00:38Z

auditlog/diff.py

@@ -189,3 +192,25 @@ def model_instance_diff(
        diff = None

    return diff
+
+
+def _changes_func() -> Callable[["LogEntry"], Dict]:


Isn't it better to move these codes to models.py?

hramezani · 2023-01-16T11:04:40Z

auditlog/management/commands/auditlogmigratejson.py

+            help="If provided, we will use native db operations.",
+            dest="db",
+            type=str,
+            choices=["postgres", "mysql", "oracle", "sqllite3"],


I think we don't support sqllite3

Also it's sqlite3, single l.

Oh. I saw it included in #407 (comment) and I thought it was supported.

aqeelat · 2023-01-17T09:45:42Z

@hramezani would you be able to write the documentation for it? Maybe in a "migration to version 3 guide" in the docs, or as a readme section.

I could write the bullet points as a comment to this PR but I'm not familiar with reStructuredText enough to write a good document.

Also, how do you recommend we write the tests for this pr?

hramezani · 2023-01-17T12:05:46Z

@hramezani would you be able to write the documentation for it? Maybe in a "migration to version 3 guide" in the docs, or as a readme section.

I could write the bullet points as a comment to this PR but I'm not familiar with reStructuredText enough to write a good document.

Ok, Please write them down in this PR and I will take care of documentation later

Also, how do you recommend we write the tests for this pr?

We need some tests for the new changes_func
We need to test the new management command as well.

Please consider to add them in separate class to be able to remove them easily later or add some note regarding the removal

aqeelat · 2023-01-19T21:59:38Z

I added a few tests but I'll come back later to polish them and add the changelog entry

codecov · 2023-01-19T22:01:25Z

Codecov Report

Merging #495 (1a3ebca) into master (f2591e4) will increase coverage by 0.37%.
The diff coverage is 98.90%.

@@            Coverage Diff             @@
##           master     #495      +/-   ##
==========================================
+ Coverage   94.56%   94.93%   +0.37%     
==========================================
  Files          30       31       +1     
  Lines         902      988      +86     
==========================================
+ Hits          853      938      +85     
- Misses         49       50       +1

Files Changed	Coverage Δ
auditlog/migrations/0015_alter_logentry_changes.py	`90.00% <88.88%> (-10.00%)`	⬇️
auditlog/apps.py	`100.00% <100.00%> (ø)`
auditlog/conf.py	`100.00% <100.00%> (ø)`
...uditlog/management/commands/auditlogmigratejson.py	`100.00% <100.00%> (ø)`
auditlog/models.py	`90.37% <100.00%> (+0.60%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

aqeelat · 2023-01-21T20:58:35Z

Now that I think of it, should I attach this PR to the current changelog entry?
Also, please review the comments I added to the pr

hramezani · 2023-01-23T12:04:01Z

auditlog/management/commands/auditlogmigratejson.py

+        parser.add_argument(
+            "-b",
+            "--bactch-size",
+            default=None,


isn't it better to have a default batch size?
I assume if the batch size is None it will process all the records in one shot am afraid it put pressure on databases with a lot of records.

Batching here is use to save memory not reduce the transaction size because bulk_update does batching by default. So, instead of storing a billion record in an array and then uploading batch by batch, passing -b will get each batch, process it, garbage collect it, then repeat.

But good comment. I added a default batch size of 50, with the option of not using batches.

hramezani · 2023-01-23T12:06:34Z

auditlog/management/commands/auditlogmigratejson.py

+            "-b",
+            "--bactch-size",
+            default=None,
+            help="Split the migration into multiple batches",


Please mention that this works just in case -d is not provided.

hramezani · 2023-01-23T12:07:53Z

auditlog/migrations/0015_alter_logentry_changes.py

+        ("auditlog", "0014_logentry_cid"),
+    ]
+
+    atomic = not settings.AUDITLOG_TWO_STEP_MIGRATION


What is the usage of this line here?

What I knew was that if atomic=True (which is the default), we won't be able to rename the field type and then create another field with the same name in the same migration because they will be run in a transaction.
But after your comment, I tested it out and it worked regardless.

hramezani · 2023-01-23T12:09:19Z

auditlog/apps.py

+
+        from auditlog import models
+
+        models.changes_func = models._changes_func()


I would move it to auditlog/models.py

When I had it in the models, it was throwing an error saying that the conf is not found. Do you have any suggestions on how to fix it?

hramezani · 2023-01-23T12:54:59Z

@aqeelat Thanks for your update.

Now that I think of it, should I attach this PR to the current changelog entry?

Yes

Also, please review the comments I added to the pr

It's because of your get_logs query. at the beginning we have L1, L2 and L3. the migration uses the first page of get_log which is L1. then in next loop, get_logs query result is L2 and L3(because L1 is migrated). So in the second loop migration picks page 2 from L2 and L3 wich is L3.
For fixing the problem I think you need to don't use paginator and always pick the first batch_size items of get_logs

aqeelat · 2023-01-23T20:17:54Z

It's because of your get_logs query. at the beginning we have L1, L2 and L3. the migration uses the first page of get_log which is L1. then in next loop, get_logs query result is L2 and L3(because L1 is migrated). So in the second loop migration picks page 2 from L2 and L3 wich is L3. For fixing the problem I think you need to don't use paginator and always pick the first batch_size items of get_logs

Good catch. I didn't know that the paginator reevaluates the qs in each iteration. I replaced it with a for-loop and it seems to be working fine now.

aqeelat · 2023-01-23T20:21:40Z

@hramezani I updated the PR with the comments you suggested.

hramezani · 2023-01-25T13:24:36Z

Thanks @aqeelat LGTM 👍

Now, you need to write down the required steps for migration (as a comment to this PR). Then I will create a PR out of them later.

aqeelat · 2023-01-26T11:20:47Z

Version 3.0.0 introduces breaking changes. Please review the migration guide below before upgrading.
If you're new to django-auditlog, you can ignore this part.

The major change in the version is that we're finally storing changes as json instead of json-text. To convert the existing records, this version has a database migration that does just that. However, this migration will take a long time if you have a huge amount of records, causing your database and application to be out of sync until the migration is complete.

To avoid this, follow these steps:

Before upgrading the package, add these two variables to settings.py:
- AUDITLOG_TWO_STEP_MIGRATION = True
- AUDITLOG_USE_TEXT_CHANGES_IF_JSON_IS_NOT_PRESENT = True
Upgrade the package. Your app will now start storing new records as JSON, but the old records will accessible via LogEntry.changes_text.
Use the newly added auditlogmigratejson command to migrate your records. Run django-admin auditlogmigratejson --help to get more information.
Once all records are migrated, remove the variables listed above, or set their values to False.

aqeelat · 2023-01-26T11:53:40Z

Feel free to reword this as you see fit.
@hramezani

hramezani · 2023-01-26T12:52:08Z

Thanks @aqeelat

I've created issue #503 for this.

I am going to test the management command in my machine and will merge it if everything works fine.
This will happen probably next week

aqeelat · 2023-04-14T08:28:48Z

Any updates here? @hramezani

hramezani · 2023-04-14T08:36:16Z

@aqeelat Not yet!

I am too busy nowadays and as this is required for V3, it's not very urgent for me.
I also need to add documentation for this.

aqeelat · 2023-04-14T10:08:23Z

Can you at least release the cid bits as a 2.x version?Sent from my iPhoneOn Apr 14, 2023, at 11:36 AM, Hasan Ramezani ***@***.***> wrote: @aqeelat Not yet! I am too busy nowadays and as this is required for V3, it's not very urgent for me. I also need to add documentation for this. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

hramezani · 2023-04-14T10:51:23Z

@aqeelat I am ok to release it in V2, we need to:

Cherry-pick cid commit on top of V2
Make a new release for V2(V2.3)

Would you like to work on it?

aqeelat · 2023-08-10T11:06:31Z

@hramezani When would you be able to approve the PR?

hramezani · 2023-08-10T11:18:02Z

@hramezani When would you be able to approve the PR?

@aqeelat we need to document how to do the migration. You already provided the required step for that and we have an issue.
My initial plan was to work on documentation for this PR but I am too busy :(
It would be great if you add the documentation if you have time, them we are good to go

aqeelat · 2023-08-10T11:27:17Z

@hramezani I'll get something out this weekend ISA.

hramezani · 2023-08-10T11:30:01Z

@hramezani I'll get something out this weekend ISA.

Great! Please add the documentation to this PR

aqeelat · 2023-08-10T19:06:15Z

@hramezani pushed

hramezani · 2023-08-11T07:16:30Z

auditlog_tests/test_two_step_json_migration.py

+        log_entry.refresh_from_db()
+
+        # Assert
+        msg = "Not yet implemented. Run this management commad without passing a -d/--database argument."


Suggested change

msg = "Not yet implemented. Run this management commad without passing a -d/--database argument."

msg = "Not yet implemented. Run this management command without passing a -d/--database argument."

aqeelat requested review from hramezani and aleh-rymasheuski January 12, 2023 14:47

aqeelat force-pushed the migration_workaround branch from 41b3c7d to 42b868b Compare January 12, 2023 14:51

aqeelat mentioned this pull request Jan 12, 2023

Workaround for the json migration caused by #407 #490

Open

hramezani reviewed Jan 16, 2023

View reviewed changes

aqeelat force-pushed the migration_workaround branch from 9b8a6cc to fe8985f Compare January 19, 2023 21:59

aqeelat force-pushed the migration_workaround branch 2 times, most recently from a114d01 to 2fe9614 Compare January 21, 2023 20:55

hramezani reviewed Jan 23, 2023

View reviewed changes

hramezani mentioned this pull request Jan 26, 2023

Add documentation for json migration asyncly #503

Closed

aqeelat force-pushed the migration_workaround branch from 890a3d8 to 4725550 Compare August 10, 2023 19:05

aqeelat force-pushed the migration_workaround branch from 4725550 to 1ca495e Compare August 10, 2023 19:07

hramezani previously approved these changes Aug 11, 2023

View reviewed changes

hramezani reviewed Aug 11, 2023

View reviewed changes

feat: give users the option to run the json migration asyncly

1a3ebca

aqeelat dismissed hramezani’s stale review via 1a3ebca August 13, 2023 08:45

aqeelat force-pushed the migration_workaround branch from 1ca495e to 1a3ebca Compare August 13, 2023 08:45

hramezani approved these changes Aug 13, 2023

View reviewed changes

hramezani merged commit 134ef73 into jazzband:master Aug 13, 2023
6 checks passed

aqeelat deleted the migration_workaround branch August 13, 2023 11:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: give users the option to run the json migration asyncly #495

feat: give users the option to run the json migration asyncly #495

aqeelat commented Jan 12, 2023

hramezani commented Jan 16, 2023

hramezani Jan 16, 2023

hramezani Jan 16, 2023

aleh-rymasheuski Jan 16, 2023

aqeelat Jan 17, 2023

aqeelat commented Jan 17, 2023

hramezani commented Jan 17, 2023

aqeelat commented Jan 19, 2023

codecov bot commented Jan 19, 2023 •

edited

Loading

aqeelat commented Jan 21, 2023

hramezani Jan 23, 2023

aqeelat Jan 23, 2023

hramezani Jan 23, 2023

hramezani Jan 23, 2023

aqeelat Jan 23, 2023 •

edited

Loading

hramezani Jan 23, 2023

aqeelat Jan 23, 2023

hramezani commented Jan 23, 2023

aqeelat commented Jan 23, 2023

aqeelat commented Jan 23, 2023

hramezani commented Jan 25, 2023

aqeelat commented Jan 26, 2023 •

edited

Loading

aqeelat commented Jan 26, 2023

hramezani commented Jan 26, 2023

aqeelat commented Apr 14, 2023

hramezani commented Apr 14, 2023

aqeelat commented Apr 14, 2023 via email

hramezani commented Apr 14, 2023

aqeelat commented Aug 10, 2023

hramezani commented Aug 10, 2023

aqeelat commented Aug 10, 2023

hramezani commented Aug 10, 2023

aqeelat commented Aug 10, 2023

hramezani Aug 11, 2023


		from auditlog import models

		models.changes_func = models._changes_func()

	msg = "Not yet implemented. Run this management commad without passing a -d/--database argument."
	msg = "Not yet implemented. Run this management command without passing a -d/--database argument."

feat: give users the option to run the json migration asyncly #495

feat: give users the option to run the json migration asyncly #495

Conversation

aqeelat commented Jan 12, 2023

hramezani commented Jan 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aqeelat commented Jan 17, 2023

hramezani commented Jan 17, 2023

aqeelat commented Jan 19, 2023

codecov bot commented Jan 19, 2023 • edited Loading

Codecov Report

aqeelat commented Jan 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aqeelat Jan 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hramezani commented Jan 23, 2023

aqeelat commented Jan 23, 2023

aqeelat commented Jan 23, 2023

hramezani commented Jan 25, 2023

aqeelat commented Jan 26, 2023 • edited Loading

aqeelat commented Jan 26, 2023

hramezani commented Jan 26, 2023

aqeelat commented Apr 14, 2023

hramezani commented Apr 14, 2023

aqeelat commented Apr 14, 2023 via email

hramezani commented Apr 14, 2023

aqeelat commented Aug 10, 2023

hramezani commented Aug 10, 2023

aqeelat commented Aug 10, 2023

hramezani commented Aug 10, 2023

aqeelat commented Aug 10, 2023

Choose a reason for hiding this comment

codecov bot commented Jan 19, 2023 •

edited

Loading

aqeelat Jan 23, 2023 •

edited

Loading

aqeelat commented Jan 26, 2023 •

edited

Loading