-
Notifications
You must be signed in to change notification settings - Fork 0
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
task/WG-141: Fix db connections for celery workers #140
Conversation
@@ -62,7 +62,7 @@ def handle_streetview_auth_exception(error: Exception): | |||
def handle_streetview_limit_exception(error: Exception): | |||
return {'message': 'Exceed concurrent streetview publish limit'}, 403 | |||
|
|||
|
|||
# ensure SQLAlchemy sessions are properly closed at the end of each request. |
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.
FYI: not directly related to the changes in this PR, but this is how the db sessions are ensured to be closed properly after each request. I'm not certain but something similar in celery task might be needed.
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.
LGTM
Tasks use a contenxt manager `create_task_session` that ensures session is closed after using. Unit tests then use db_session for testing. db_session is also used when processing reequests in flask.
Uploading of files is no longer used.
|
||
@api.doc(id="uploadPointCloud", |
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 the only large out-of-scope change. I drop this endpoint which allows point clouds to be directly uploaded in a POST. This endpoint is NOT being used by frontend. Instead of uploading a file,the current frontends allow users to select to import an existing pointcloud on DesignSafe and then the backend gets the file on their behalf and processes it.
We have some other non-used endpoints where users can upload file directly but none of the frontends use those anymore. All the frontends use the endpoint where users say which file from DS Tapis we should import for them.
Really, we should probably drop all the upload routs.
I'm dropping this one here as it made refactoring the use of database sessions by point cloud tasks and methods MUCH easier.
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.
LGTM! Thanks for the detail on the 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.
LGTM (retroactively).
Overview:
Disable pool connections for celery workers. This is a temp fix/workaround for our connection related issues.This PR refactors how database session is used in celery worker tasks. Now,
create_task_session
is a context manger that provides a session and then ensure that session is closed after its used (and rolled-back if there was an exception).Tasks look like the following:
So going forward,
db_session
from db.py is a scoped_session that is used when processing requests in Flask and also in unit tests for fixtures and asserts. Then for all celery tasks, thecreate_task_session
is used.PR Status:
Related Jira tickets: