Move metadata cache one layer up #6153

youngyjd · 2018-10-19T21:26:54Z

pyhive is calling use default when a connection is initiated, https://github.com/dropbox/PyHive/blob/2c2446bf905ea321aac9dcdd3fa033909ff0b0b5/pyhive/hive.py#L205. This statement took 3 seconds in my experiment, so even metadata cache is enabled for Hive, user experience is still not very good.

Moving the cache one layer up can avoid running use default statement.

Refactored the cache logic for multi schema metadata fetch. Every time when multi schema metadata fetch is updated, it refreshes table list cache for every schema as well.

codecov-io · 2018-10-24T06:01:26Z

Codecov Report

Merging #6153 into master will increase coverage by 0.03%.
The diff coverage is 40.47%.

@@            Coverage Diff             @@
##           master    #6153      +/-   ##
==========================================
+ Coverage   76.92%   76.95%   +0.03%     
==========================================
  Files          47       47              
  Lines        9388     9375      -13     
==========================================
- Hits         7222     7215       -7     
+ Misses       2166     2160       -6

Impacted Files	Coverage Δ
superset/cli.py	`35.85% <0%> (-0.15%)`	⬇️
superset/config.py	`92.36% <100%> (ø)`	⬆️
superset/db_engine_specs.py	`55.45% <15.38%> (-0.72%)`	⬇️
superset/views/core.py	`73.86% <27.27%> (-0.27%)`	⬇️
superset/models/core.py	`84.35% <48.64%> (+0.47%)`	⬆️
superset/utils/cache.py	`62.5% <64.28%> (+14.35%)`	⬆️
superset/connectors/sqla/models.py	`80.71% <0%> (-0.24%)`	⬇️
superset/security.py	`75.67% <0%> (+0.11%)`	⬆️

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 e21e6a7...b42a404. Read the comment docs.

youngyjd · 2018-10-24T17:59:39Z

@betodealmeida @mistercrunch Please take a look

betodealmeida · 2018-10-26T00:26:09Z

superset/models/core.py

+        """
+        if not self.allow_multi_schema_metadata_fetch:
+            return []
+        tables_dict = self.db_engine_spec.fetch_result_sets(self, 'table')


The result from the method fetch_results_sets is super confusing... took me a while to understand line 879.

I was feeling confused as well. let me see whether I can refactor it as well.

betodealmeida · 2018-10-26T00:27:49Z

superset/cli.py

+            print('Fetching {} datasources ...'.format(database.name))
+            try:
+                database.all_table_names_in_database(
+                    db_id=database.id, force=True,


Why do you need to pass database.id here, if this is a method of database? Looking at other places where this method is called, looks like db_id is always the same as database.id, no?

betodealmeida · 2018-10-26T00:46:38Z

superset/models/core.py

+                                    cache_timeout=None, force=False):
+        """
+        Parameters need to be passed as keyword arguments.
+        If cache=True, db_id must be passed in for setting cache key


Oh, you want the database id to be part of the key. In that case, I think it's better to create a new decorator reusing cache_util.memoized_func, that extracts the id from self. Here's a simple example showing the technique:

def instance_memo(attribute, cache={}): def decorator(func): def decorated(self, *args, **kwargs): # in this example, key is just `Database.id` key = getattr(self, attribute) if key not in cache: print('Writing to cache') cache[key] = func(self, *args, **kwargs) else: print('Reading from cache') return cache[key] return decorated return decorator class Database: def __init__(self, id): self.id = id @instance_memo('id') def method(self): pass

Calling it we get:

>>> db1 = Database(1) >>> db1.method() Writing to cache >>> db1.method() Reading from cache >>> db1.method() Reading from cache >>> db2 = Database(2) >>> db2.method() Writing to cache >>> db2.method() Reading from cache >>> db2.method() Reading from cache

betodealmeida · 2018-10-26T00:47:59Z

superset/models/core.py

+                inspector=self.inspector, schema=schema)
+        except Exception as e:
+            logging.exception(e)
+            pass


You don't need pass here, you only need it in empty blocks (eg, if line 927 didn't have the logging).

betodealmeida · 2018-10-26T00:48:23Z

superset/models/core.py

-        except Exception:
+                inspector=self.inspector, schema=schema)
+        except Exception as e:
+            logging.exception(e)


Same here, you can remove pass below.

youngyjd · 2018-10-26T23:22:09Z

@betodealmeida Comments addressed. Please take another look.

betodealmeida · 2018-10-31T20:23:22Z

Awesome! Thanks, @youngyjd!

betodealmeida · 2018-10-31T19:09:16Z

superset/config.py

-CACHE_CONFIG = {'CACHE_TYPE': 'null'}
-TABLE_NAMES_CACHE_CONFIG = {'CACHE_TYPE': 'null'}
+CACHE_CONFIG = {'CACHE_TYPE': 'simple'}
+TABLE_NAMES_CACHE_CONFIG = {'CACHE_TYPE': 'simple'}


I assume you changed this for testing — can you revert the change?

You are right! My bad

* Update wording * nit update for api endpoint url * move metadata cache one layer up * refactor cache * fix flake8 and DatabaseTablesAsync * nit * remove logging for cache * only fetch for all tables that allows cross schema fetch * default allow_multi_schema_metadata_fetch to False * address comments * remove unused defaultdict * flake 8 (cherry picked from commit c552c12)

* Update wording * nit update for api endpoint url * move metadata cache one layer up * refactor cache * fix flake8 and DatabaseTablesAsync * nit * remove logging for cache * only fetch for all tables that allows cross schema fetch * default allow_multi_schema_metadata_fetch to False * address comments * remove unused defaultdict * flake 8

youngyjd changed the title ~~Update wording~~ Move metadata cache one layer up Oct 24, 2018

youngyjd force-pushed the update-database-configuration-instruction branch 2 times, most recently from 994f50b to 199c7b5 Compare October 24, 2018 05:49

youngyjd force-pushed the update-database-configuration-instruction branch 3 times, most recently from fe747fb to 214eaa1 Compare October 25, 2018 20:12

youngyjd added 9 commits October 25, 2018 13:48

Update wording

d158aba

nit update for api endpoint url

fec0311

move metadata cache one layer up

e2ef37e

refactor cache

3de929c

fix flake8 and DatabaseTablesAsync

4843d43

nit

2642b94

remove logging for cache

22197c3

only fetch for all tables that allows cross schema fetch

800297e

default allow_multi_schema_metadata_fetch to False

06b3a25

youngyjd force-pushed the update-database-configuration-instruction branch from 1fd8623 to 06b3a25 Compare October 25, 2018 20:48

betodealmeida reviewed Oct 26, 2018

View reviewed changes

youngyjd added 2 commits October 26, 2018 16:15

address comments

fe13b4c

remove unused defaultdict

b32a149

flake 8

b42a404

betodealmeida merged commit c552c12 into apache:master Oct 31, 2018

betodealmeida reviewed Oct 31, 2018

View reviewed changes

youngyjd mentioned this pull request Nov 1, 2018

revert the change in config.py #6255

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move metadata cache one layer up #6153

Move metadata cache one layer up #6153

youngyjd commented Oct 19, 2018 •

edited

codecov-io commented Oct 24, 2018 •

edited

youngyjd commented Oct 24, 2018

betodealmeida Oct 26, 2018

youngyjd Oct 26, 2018

betodealmeida Oct 26, 2018

betodealmeida Oct 26, 2018

betodealmeida Oct 26, 2018

betodealmeida Oct 26, 2018

youngyjd commented Oct 26, 2018

betodealmeida commented Oct 31, 2018

betodealmeida Oct 31, 2018

youngyjd Nov 1, 2018

Move metadata cache one layer up #6153

Move metadata cache one layer up #6153

Conversation

youngyjd commented Oct 19, 2018 • edited

codecov-io commented Oct 24, 2018 • edited

Codecov Report

youngyjd commented Oct 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngyjd commented Oct 26, 2018

betodealmeida commented Oct 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngyjd commented Oct 19, 2018 •

edited

codecov-io commented Oct 24, 2018 •

edited