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

fix: added scoped session for SQL-based alerting #10531

Closed
wants to merge 2 commits into from

Conversation

JasonD28
Copy link
Contributor

@JasonD28 JasonD28 commented Aug 6, 2020

SUMMARY

Multiple errors such as sqlalchemy.exc.NoSuchColumnError, sqlalchemy.exc.ResourceClosedError and MySQLdb._exceptions.OperationalError were being seen when multiple alert celery tasks were run concurrently. These errors occurred during the first few iterations of a celery beat schedule after initialization, however they would stop after after a few iterations of the beat schedule. This PR uses db.create_scoped_session() for alerting celery tasks to fix these errors. Retrying is implemented along with the celery task as a temporary fix for #10530

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

  • local test
  • dropbox staging
  • dropbox prod

ADDITIONAL INFORMATION

@villebro
Copy link
Member

villebro commented Aug 6, 2020

FYI @john-bodley , interesting empirical observations for when scoped sessions can be helpful. This might explain the presence of some of the scoped sessions that were removed in #10427

@villebro
Copy link
Member

villebro commented Aug 6, 2020

@JasonD28 Did this change solve the errors completely?

@JasonD28
Copy link
Contributor Author

JasonD28 commented Aug 6, 2020

@villebro yes it did, but a better solution than retrying would be ideal to solve #10530

@villebro
Copy link
Member

villebro commented Aug 6, 2020

I'm trying to determine what the effect of scoped sessions vs vanilla sessions is. Did you try with scoped sessions without the retry, did that improve reliability?

@JasonD28
Copy link
Contributor Author

JasonD28 commented Aug 6, 2020

scoped sessions without retry did improve reliability with the exception of #10530

for reference, before adding scoped sessions sqlalchemy.exc.NoSuchColumnError, sqlalchemy.exc.ResourceClosedError and MySQLdb._exceptions.OperationalError were seen at these places: https://github.com/apache/incubator-superset/blob/ece91928a9339190163c0bc72b96e51217a90d1e/superset/tasks/schedules.py#L544 https://github.com/apache/incubator-superset/blob/ece91928a9339190163c0bc72b96e51217a90d1e/superset/tasks/schedules.py#L570 https://github.com/apache/incubator-superset/blob/ece91928a9339190163c0bc72b96e51217a90d1e/superset/tasks/schedules.py#L626

@villebro
Copy link
Member

villebro commented Aug 6, 2020

@JasonD28 thanks for the context, really helpful! 👍

@john-bodley
Copy link
Member

@villebro thanks for being looped in. I do wonder if the scoped session is merely masking some other issue we haven’t addresses, i.e., it seems somewhat nonintuitive that a scoped session is necessary here.

@JasonD28
Copy link
Contributor Author

JasonD28 commented Aug 6, 2020

@villebro @john-bodley further testing showed that just implementing retrying was sufficient enough for the celery task to run without any errors. While non-scoped sessions invoked retrying more often than scoped sessions, I'm going to close this PR and move forward with non-scoped sessions #10542 until more information is know about whats causing the error.

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

Successfully merging this pull request may close these issues.

4 participants