-
Notifications
You must be signed in to change notification settings - Fork 221
Add Django connection pools feature #5332
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
Conversation
facc61b
to
7af1c09
Compare
Co-authored-by: Dhruv Bhanushali <[email protected]>
70c61d9
to
b9b9e63
Compare
Latest k6 run output1
Footnotes
|
b9b9e63
to
9e5d05f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! It's truly wild that the two kinds of tests were mutually interfering and we hadn't caught this so far.
api/justfile
Outdated
# When the unit tests run concurrently with the integration tests, the integration tests fail | ||
# because the unit tests drop the real database in the end. This is a workaround to run the unit | ||
# tests first and then the integration tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nit: a single-line comment here would be preferable as this will be shown when running just
with no arguments. The long one can be inside the recipe.
1f5a61c
to
acfc339
Compare
Description
Adding connection pools to fix the memory leak and improve the db connections.
Some tests were fixed by adding
(transaction=True)
to@pytest.mark.django_db
.Somehow, the handling of the database in unit tests was interfering with the real database in the integration tests, because without the
transaction
settings on the unit tests that were handling user/throttled_app tables, the integration auth test was failing. I think the rolling back of the database that happens with the default setting(transaction=False)
was interfering with the main database.To minimize the interaction between the unit test mock database and the integration tests database, I added a
just api/test-ci
script that runs unit tests before running the integration tests.There were two tests that were marked as skipped, as well as all tests for the examples.@krysal, @dhruvkb, do you think it's okay to skip the tests and try this feature in prod? Or should we try to get all of the tests to pass (in that case, we should probably have in sync session for it because I don't know how to fix the last tests).
Testing Instructions
The CI should pass and the number of db connections should not keep increasing in production after the release.
Checklist
Update index.md
).main
) or a parent feature branch.ov just catalog/generate-docs
for catalogPRs) or the media properties generator (
ov just catalog/generate-docs media-props
for the catalog or
ov just api/generate-docs
for the API) where applicable.Developer Certificate of Origin
Developer Certificate of Origin