-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
[Feature] enhanced memoized on get_sqla_engine and other functions #3530
[Feature] enhanced memoized on get_sqla_engine and other functions #3530
Conversation
2 similar comments
superset/models/core.py
Outdated
@@ -590,6 +590,8 @@ def set_sqlalchemy_uri(self, uri): | |||
conn.password = password_mask if conn.password else None | |||
self.sqlalchemy_uri = str(conn) # hides the password | |||
|
|||
@utils.memoized( |
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 watch
is a nice precaution (keep it!), but I think that these attrs wouldn't change during the lifecycle of the ORM instance object. Unless I'm missing something they'd live over a single web request or CLI command and get garbage collected.
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.
That's what I thought as well, but without watch
a good number of unit tests were failing/in error. Though it might just be that the tests modifying attributes instead of creating new objects to set up the tests.
Oops merge conflict. Otherwise LGTM. Thanks for following through on this. |
Somehow travis is stuck and I cannot merge if the build doesn't pass... Please re-trigger the build by adding a dummy commit on top |
Closing/reopening is weird but slightly more reliable :P |
@Mogball if you push stuff to a branch you don't need to close and reopen it to have CI start again, |
2 similar comments
3 similar comments
f45b6bb
to
3d86da1
Compare
3d86da1
to
9e58b89
Compare
@mistercrunch Just want to bump this before it falls into oblivion |
…pache#3530) * added watch to memoized * added unit tests for memoized * code style changes
…pache#3530) * added watch to memoized * added unit tests for memoized * code style changes
Added
@memoized
to some model functions, i.e.get_sqla_engine()
Also modified
@memoized
to take an argumentwatch
which is a tuple of the names of instance attributes that the decorator should check along with the method arguments.@memoized
withoutwatch
works as usual.