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

[Feature] enhanced memoized on get_sqla_engine and other functions #3530

Merged

Conversation

Mogball
Copy link
Contributor

@Mogball Mogball commented Sep 26, 2017

Added @memoized to some model functions, i.e. get_sqla_engine()

Also modified @memoized to take an argument watch which is a tuple of the names of instance attributes that the decorator should check along with the method arguments.

@memoized without watch works as usual.

@Mogball Mogball changed the title Feature: memoized instance methods Feature: memoized get_sqla_engine and some others Sep 26, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 69.688% when pulling 787e229 on Mogball:mogball/feature/memoized_functions into 8efcaeb on apache:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 69.688% when pulling 787e229 on Mogball:mogball/feature/memoized_functions into 8efcaeb on apache:master.

@coveralls
Copy link

coveralls commented Sep 26, 2017

Coverage Status

Coverage increased (+0.1%) to 69.688% when pulling 787e229 on Mogball:mogball/feature/memoized_functions into 8efcaeb on apache:master.

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

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.

Copy link
Contributor Author

@Mogball Mogball Sep 26, 2017

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.

@mistercrunch
Copy link
Member

Oops merge conflict. Otherwise LGTM. Thanks for following through on this.

@mistercrunch
Copy link
Member

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

@Mogball Mogball closed this Sep 28, 2017
@Mogball Mogball reopened this Sep 28, 2017
@coveralls
Copy link

coveralls commented Sep 28, 2017

Coverage Status

Coverage increased (+0.3%) to 70.255% when pulling 2049d57 on Mogball:mogball/feature/memoized_functions into 7d934e7 on apache:master.

@Mogball
Copy link
Contributor Author

Mogball commented Sep 28, 2017

Closing/reopening is weird but slightly more reliable :P

@Mogball Mogball changed the title Feature: memoized get_sqla_engine and some others [Feature] enhanced memoized on get_sqla_engine and other functions Oct 4, 2017
@Mogball Mogball closed this Oct 10, 2017
@Mogball Mogball reopened this Oct 10, 2017
@xrmx
Copy link
Contributor

xrmx commented Oct 10, 2017

@Mogball if you push stuff to a branch you don't need to close and reopen it to have CI start again,

@coveralls
Copy link

coveralls commented Oct 10, 2017

Coverage Status

Coverage increased (+0.09%) to 70.163% when pulling 8e16b19 on Mogball:mogball/feature/memoized_functions into d7f8a7f on apache:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 70.163% when pulling 8e16b19 on Mogball:mogball/feature/memoized_functions into d7f8a7f on apache:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 70.163% when pulling 8e16b19 on Mogball:mogball/feature/memoized_functions into d7f8a7f on apache:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 71.36% when pulling 64398ba on Mogball:mogball/feature/memoized_functions into 200b66d on apache:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 71.36% when pulling 64398ba on Mogball:mogball/feature/memoized_functions into 200b66d on apache:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 71.36% when pulling 64398ba on Mogball:mogball/feature/memoized_functions into 200b66d on apache:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 71.36% when pulling 64398ba on Mogball:mogball/feature/memoized_functions into 200b66d on apache:master.

@Mogball Mogball force-pushed the mogball/feature/memoized_functions branch from f45b6bb to 3d86da1 Compare December 16, 2017 09:22
@Mogball Mogball force-pushed the mogball/feature/memoized_functions branch from 3d86da1 to 9e58b89 Compare December 16, 2017 09:26
@Mogball
Copy link
Contributor Author

Mogball commented Dec 16, 2017

@mistercrunch Just want to bump this before it falls into oblivion

@mistercrunch mistercrunch merged commit af7cdeb into apache:master Dec 17, 2017
michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018
…pache#3530)

* added watch to memoized

* added unit tests for memoized

* code style changes
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
…pache#3530)

* added watch to memoized

* added unit tests for memoized

* code style changes
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.23.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.23.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants