Skip to content

Conversation

@ktmud
Copy link
Member

@ktmud ktmud commented Oct 21, 2020

SUMMARY

This fixes a bug where when DASHBOARD_CACHE feature flag is enabled, saving datasource will become too slow and sometimes generate the following error:

(raised as a result of Query-invoked autoflush; consider using a session.no_autoflush block if this flush is occurring prematurely) (pymysql.err.OperationalError) (1205, 'Lock wait timeout exceeded; try restarting transaction') [SQL: UPDATE table_columns SET changed_on=%(changed_on)s, is_active=%(is_active)s, changed_by_fk=%(changed_by_fk)s WHERE table_columns.id = %(table_columns_id)s] [parameters: {'changed_on': datetime.datetime(2020, 10, 20, 17, 15, 11, 547150), 'is_active': None, 'changed_by_fk': 9117, 'table_columns_id': 1555231}] (Background on this error at: http://sqlalche.me/e/13/e3q8)

This is because of a wrong join condition where I thought multiple join conditions can be passed to SQLAlchemy's join as consecutive positional arguments, when it's actually expecting only one positional argument for the join conditions.

Also fixes a typo that has made editing datasource unable to update the dashboard cache.

cc @graceguo-supercat @serenajiang

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TEST PLAN

  • Enable DASHBOARD_CACHE
  • Try edit a datasource in the Explore view

ADDITIONAL INFORMATION

Copy link

@graceguo-supercat graceguo-supercat left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov-io
Copy link

codecov-io commented Oct 21, 2020

Codecov Report

Merging #11369 into master will decrease coverage by 7.18%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11369      +/-   ##
==========================================
- Coverage   65.77%   58.58%   -7.19%     
==========================================
  Files         838      800      -38     
  Lines       39841    38513    -1328     
  Branches     3655     3443     -212     
==========================================
- Hits        26206    22564    -3642     
- Misses      13534    15779    +2245     
- Partials      101      170      +69     
Flag Coverage Δ
#cypress 56.00% <ø> (+<0.01%) ⬆️
#javascript ?
#python 59.99% <ø> (-0.96%) ⬇️

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

Impacted Files Coverage Δ
superset/models/dashboard.py 80.63% <ø> (ø)
...uperset-frontend/src/dashboard/util/dnd-reorder.js 0.00% <0.00%> (-100.00%) ⬇️
...rset-frontend/src/dashboard/util/getEmptyLayout.js 0.00% <0.00%> (-100.00%) ⬇️
...et-frontend/src/components/Menu/LanguagePicker.tsx 0.00% <0.00%> (-100.00%) ⬇️
...dashboard/components/resizable/ResizableHandle.jsx 0.00% <0.00%> (-100.00%) ⬇️
.../src/dashboard/util/getFilterScopeFromNodesTree.js 0.00% <0.00%> (-93.48%) ⬇️
...uperset-frontend/src/utils/getClientErrorObject.ts 0.00% <0.00%> (-89.19%) ⬇️
.../src/dashboard/components/FilterIndicatorGroup.jsx 11.76% <0.00%> (-88.24%) ⬇️
...c/explore/components/controls/withVerification.jsx 9.09% <0.00%> (-87.88%) ⬇️
...src/dashboard/components/gridComponents/Header.jsx 10.52% <0.00%> (-86.85%) ⬇️
... and 282 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 54c2ad4...73f5cc0. Read the comment docs.

Comment on lines +317 to +318
(Slice.id == dashboard_slices.c.slice_id)
& (Slice.datasource_id == datasource_id),
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think this could be expressed as and_(Slice.id == dashboard_slices.c.slice_id, Slice.datasource_id == datasource_id)

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know, I kind of like the bit operator better..

@ktmud ktmud changed the title bugfix: dashboard cache invalid join query fix: dashboard cache invalid join query Oct 22, 2020
@ktmud ktmud merged commit b89a5d6 into apache:master Oct 22, 2020
@ktmud ktmud deleted the dashboard-cache branch October 22, 2020 19:48
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
* bugfix: dashboard cache invalid join query

* Use engine instead of session
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.0 First shipped in 1.0.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 size/S 🚢 1.0.0 First shipped in 1.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants