-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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
allow cache and force refresh on table list #6078
Conversation
d516d61
to
853d874
Compare
if (!(db)) { | ||
this.setState({ tableOptions: [] }); | ||
} else { | ||
this.fetchTables(val, this.props.queryEditor.schema); |
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.
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 Report
@@ 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
Continue to review full report at Codecov.
|
Please take a look @betodealmeida |
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.
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: [] }); |
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.
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" |
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.
shouldn't this say schema
?
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.
Also, don't forget i18n
as in tooltipContent={t('force refresh table list')}
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.
woops. my bad.
<RefreshLabel | ||
onClick={this.changeSchema.bind( | ||
this, { value: this.props.queryEditor.schema }, true)} | ||
tooltipContent="force refresh schema list" |
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.
shouldn't this say table list
?
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.
i18n
superset/db_engine_specs.py
Outdated
@@ -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), |
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.
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.
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.
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.
59f5818
to
c147497
Compare
@mistercrunch Please take another look. |
* allow cache and force refresh on table list * wording * flake8 * javascript test * address comments * nit (cherry picked from commit 177bed3)
* allow cache and force refresh on table list * wording * flake8 * javascript test * address comments * nit
This is a follow-up PR for #5933
table_cache_timeout
settings tometadata_cache_timeout
in database configuration. Iftable_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.