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

allow cache and force refresh on table list #6078

Conversation

youngyjd
Copy link
Contributor

@youngyjd youngyjd commented Oct 11, 2018

This is a follow-up PR for #5933

  1. Added table_cache_timeout settings to metadata_cache_timeout in database configuration. If table_cache_timeout is unset in the field, cache will not be enabled for the metadata fetch of this database. If it is set to 0, cache will never time out.
  2. Added a button to sqllab UI which allows user to refetch table list metadata, or force refresh cache if cache is enabled.

screen shot 2018-10-11 at 4 53 56 pm

@youngyjd youngyjd force-pushed the allow-cache-and-force-refresh-on-table-list branch from d516d61 to 853d874 Compare October 11, 2018 23:49
if (!(db)) {
this.setState({ tableOptions: [] });
} else {
this.fetchTables(val, this.props.queryEditor.schema);
Copy link
Contributor Author

@youngyjd youngyjd Oct 12, 2018

Choose a reason for hiding this comment

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

fetchTables here makes no sense because the database has been changed but this.props.queryEditor.schema is still the schema selected for the previously selected db.
And it is set null on line 44:
this.props.actions.queryEditorSetSchema(this.props.queryEditor, null);

so I think removing it is necessary.

@codecov-io
Copy link

codecov-io commented Oct 12, 2018

Codecov Report

Merging #6078 into master will decrease coverage by 0.32%.
The diff coverage is 45.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6078      +/-   ##
==========================================
- Coverage   77.56%   77.23%   -0.33%     
==========================================
  Files          47       47              
  Lines        9484     9349     -135     
==========================================
- Hits         7356     7221     -135     
  Misses       2128     2128
Impacted Files Coverage Δ
superset/views/core.py 74.08% <0%> (-0.11%) ⬇️
superset/models/core.py 83.87% <36.36%> (-1.24%) ⬇️
superset/cache_util.py 48.14% <50%> (-6.14%) ⬇️
superset/db_engine_specs.py 56.1% <61.53%> (+0.23%) ⬆️
superset/connectors/base/views.py 71.42% <0%> (-10.39%) ⬇️
superset/dict_import_export_util.py 21.05% <0%> (-7.52%) ⬇️
superset/legacy.py 13.43% <0%> (-4.88%) ⬇️
superset/stats_logger.py 60.97% <0%> (-3.47%) ⬇️
superset/translations/utils.py 86.66% <0%> (-2.81%) ⬇️
... and 40 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 e37b83a...b858166. Read the comment docs.

@youngyjd
Copy link
Contributor Author

Please take a look @betodealmeida

Copy link
Member

@mistercrunch mistercrunch left a comment

Choose a reason for hiding this comment

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

Few minor changes requested, otherwise LGTM

@@ -41,12 +41,10 @@ class SqlEditorLeftBar extends React.PureComponent {
onDatabaseChange(db, force) {
const val = db ? db.value : null;
this.setState({ schemaOptions: [] });
this.setState({ tableOptions: [] });
Copy link
Member

Choose a reason for hiding this comment

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

A single setState call with ({ schemaOptions: [], tableOptions: [] }) would be better as it would trigger a single render call instead of 2.

@@ -196,7 +195,7 @@ class SqlEditorLeftBar extends React.PureComponent {
<RefreshLabel
onClick={this.onDatabaseChange.bind(
this, { value: database.id }, true)}
tooltipContent="force refresh schema list"
tooltipContent="force refresh table list"
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this say schema?

Copy link
Member

Choose a reason for hiding this comment

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

Also, don't forget i18n as in tooltipContent={t('force refresh table list')}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

woops. my bad.

<RefreshLabel
onClick={this.changeSchema.bind(
this, { value: this.props.queryEditor.schema }, true)}
tooltipContent="force refresh schema list"
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this say table list?

Copy link
Member

Choose a reason for hiding this comment

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

i18n

@@ -318,9 +318,25 @@ def get_schema_names(cls, inspector, db_id,
return inspector.get_schema_names()

@classmethod
def get_table_names(cls, schema, inspector):
@cache_util.memoized_func(
enable_cache=lambda *args, **kwargs: kwargs.get('enable_cache', False),
Copy link
Member

Choose a reason for hiding this comment

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

The lambda stuff looks a bit nasty to me, had to go read the decorator docstring to figure out what is going on here. I understand this is outside the scope of this PR so it's ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored the decorator a little to avoid too many lambda functions passed to the decorator. Please let me know how I can improve it better.

@youngyjd youngyjd force-pushed the allow-cache-and-force-refresh-on-table-list branch from 59f5818 to c147497 Compare October 15, 2018 21:42
@youngyjd
Copy link
Contributor Author

@mistercrunch Please take another look.

@betodealmeida betodealmeida merged commit 177bed3 into apache:master Oct 16, 2018
youngyjd added a commit to lyft/incubator-superset that referenced this pull request Oct 17, 2018
* allow cache and force refresh on table list

* wording

* flake8

* javascript test

* address comments

* nit

(cherry picked from commit 177bed3)
youngyjd added a commit to lyft/incubator-superset that referenced this pull request Nov 2, 2018
* allow cache and force refresh on table list

* wording

* flake8

* javascript test

* address comments

* nit

(cherry picked from commit 177bed3)
(cherry picked from commit 214edcc)
youngyjd added a commit to lyft/incubator-superset that referenced this pull request Nov 2, 2018
* allow cache and force refresh on table list

* wording

* flake8

* javascript test

* address comments

* nit

(cherry picked from commit 177bed3)
(cherry picked from commit 214edcc)
bipinsoniguavus pushed a commit to ThalesGroup/incubator-superset that referenced this pull request Dec 26, 2018
* allow cache and force refresh on table list

* wording

* flake8

* javascript test

* address comments

* nit
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.34.0 labels Feb 27, 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 🚢 0.34.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants