-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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: session error fixed related to thumbnails. #12760
Conversation
Codecov Report
@@ Coverage Diff @@
## master #12760 +/- ##
==========================================
- Coverage 66.88% 66.78% -0.11%
==========================================
Files 1021 1022 +1
Lines 50015 50051 +36
Branches 4907 4915 +8
==========================================
- Hits 33455 33429 -26
- Misses 16435 16498 +63
+ Partials 125 124 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
superset/tasks/thumbnails.py
Outdated
@@ -49,6 +49,7 @@ def cache_chart_thumbnail( | |||
user = security_manager.get_user_by_username( | |||
current_app.config["THUMBNAIL_SELENIUM_USER"], session=session | |||
) | |||
user = session.merge(user) |
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.
possible that the user is not bound to a session since it's outside of the nullpool
session block and at the end of the block the session is closed, can you revert and test placing screenshot.compute_and_cache
under the session_scope
?
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.
Yeah it worked as well. I changed code accordingly.
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.
Awesome!! thank you for the fix
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.
Looks good, just lint issues that need to be solved: superset/tasks/thumbnails.py:77:0: C0303: Trailing whitespace (trailing-whitespace)
@dpgaspar Fixed |
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.
Looks good!
* fix: session error fixed related to thumbnails. * compute_and_cache moved to session scope * lint fix done
SUMMARY
Thumbnails stopped working on 1.0.0 because of unbound session of sqlaclhemy. See #12726
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
ADDITIONAL INFORMATION