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: session error fixed related to thumbnails. #12760

Merged
merged 3 commits into from
Jan 27, 2021
Merged

fix: session error fixed related to thumbnails. #12760

merged 3 commits into from
Jan 27, 2021

Conversation

iercan
Copy link
Contributor

@iercan iercan commented Jan 26, 2021

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

@codecov-io
Copy link

codecov-io commented Jan 26, 2021

Codecov Report

Merging #12760 (574fde9) into master (e8857ba) will decrease coverage by 0.10%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            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     
Flag Coverage Δ
cypress 50.87% <ø> (-0.05%) ⬇️
javascript 61.68% <ø> (+0.37%) ⬆️
python 63.74% <0.00%> (-0.36%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset/tasks/thumbnails.py 42.42% <0.00%> (ø)
superset/db_engines/hive.py 0.00% <0.00%> (-85.72%) ⬇️
superset/db_engine_specs/hive.py 73.84% <0.00%> (-17.31%) ⬇️
...ntend/src/dashboard/components/PropertiesModal.jsx 75.67% <0.00%> (-7.21%) ⬇️
superset/db_engine_specs/presto.py 81.38% <0.00%> (-7.15%) ⬇️
...et-frontend/src/components/Menu/LanguagePicker.tsx 64.28% <0.00%> (-4.95%) ⬇️
superset-frontend/src/utils/common.js 70.96% <0.00%> (-3.23%) ⬇️
...perset-frontend/src/views/CRUD/chart/ChartList.tsx 79.52% <0.00%> (-2.37%) ⬇️
superset/views/database/mixins.py 80.70% <0.00%> (-1.76%) ⬇️
...perset-frontend/src/datasource/DatasourceModal.tsx 72.13% <0.00%> (-1.64%) ⬇️
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8857ba...574fde9. Read the comment docs.

@@ -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)
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Member

@dpgaspar dpgaspar left a 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)

@iercan
Copy link
Contributor Author

iercan commented Jan 26, 2021

Looks good, just lint issues that need to be solved: superset/tasks/thumbnails.py:77:0: C0303: Trailing whitespace (trailing-whitespace)

@dpgaspar Fixed

@junlincc junlincc added the new:contributor The author is a new contributor label Jan 27, 2021
Copy link
Member

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@dpgaspar dpgaspar merged commit 49e6e42 into apache:master Jan 27, 2021
villebro pushed a commit that referenced this pull request Jan 29, 2021
* fix: session error fixed related to thumbnails.

* compute_and_cache moved to session scope

* lint fix done
@mistercrunch mistercrunch added 🍒 1.0.1 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels new:contributor The author is a new contributor size/S v1.0.1 🍒 1.0.1 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants