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

Add elapsed time in update index command #1695

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

pauloxnet
Copy link
Member

@pauloxnet pauloxnet commented Oct 24, 2024

I moved the transaction.atomic from decorator to context manager and added the elapsed time information in the command output.

I used this code to perform this check on the documentation update in the issue #1693

See #1693 (comment)

@pauloxnet pauloxnet requested review from a team October 24, 2024 20:30
@camilonova camilonova merged commit 021b2fb into django:main Oct 24, 2024
5 checks passed
@bmispelon
Copy link
Member

I'm a bit late to the party here (didn't have time to review this last night).

I don't really understand the reasoning behind moving the code from a decorator to a context manager, could you expand on that?

Since the PR had no tests (which I understand might be a little tricky to write), there's not really anything preventing someone from reverting to a decorator in the future. If this is about performance, a comment indicating that would be helpful.

@pauloxnet
Copy link
Member Author

I don't really understand the reasoning behind moving the code from a decorator to a context manager, could you expand on that?

Following the Django documentation about "Database transactions":

"Open transactions have a performance cost for your database server. To minimize this overhead, keep your transactions as short as possible."

I wanted to leave in the transaction only the two lines of code for which it made sense to have it, leaving out the calculation of the elapsed time or the writing to the terminal.

Since the PR had no tests (which I understand might be a little tricky to write), there's not really anything preventing someone from reverting to a decorator in the future. If this is about performance, a comment indicating that would be helpful.

I agree that a comment would be nice, and if you want I can add it, but the short-term goal is to convert the vector search field to a generated field (see #1650), which will allow us to completely remove the need to perform this update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants