feat: EXTRA_CACHING_STRATEGIES #16150
Conversation
Codecov Report
@@ Coverage Diff @@
## master #16150 +/- ##
==========================================
+ Coverage 76.47% 76.49% +0.01%
==========================================
Files 997 1002 +5
Lines 53245 53272 +27
Branches 6777 6777
==========================================
+ Hits 40720 40750 +30
+ Misses 12295 12292 -3
Partials 230 230
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
…gies * upstream/master: (64 commits) check roles before fetching reports (#16260) chore: upgrade mypy and add type guards (#16227) fix: pivot columns with ints for name (#16259) chore(pylint): Bump Pylint to 2.9.6 (#16146) fix examples tab for dashboard (#16253) chore: bump superset-ui packages to 0.17.84 (#16251) chore: Shows the dataset description in the gallery dropdown (#16200) fix(Dashboard): Omnibar dropdown visibility and keyboard commands (#16168) chore: bump py version for integration test (#16213) fix: skip perms on query context update (#16250) refactor: external metadata fetch API (#16193) feat(dao): admin can remove self from object owners (#15149) fix(dashboard): cross filter chart highlight when filters badge icon clicked (#16233) fix: validate_parameters and query (#16241) fix: Remove Advanced Analytics tag for 2 charts (#16240) Revert "feat: Changing Dataset names (#16199)" (#16235) feat: Allow users to connect via legacy SQLA form (#16201) fix: remove encryption from db params (#16214) fix(Explore): Show the tooltip only when label does not fit the container in METRICS/FILTERS/GROUP BY/SORT BY of the DATA panel (#16060) Show/hide tooltips (#16192) ... # Conflicts: # superset/tasks/caching/cache_strategy.py
* master: chore(pylint): Reenable ungrouped-imports check (#16256) chore(pylint): Re-enable super-with-arguments check (#16138) fix: disable text reports for now (#16257) fix: pivot col names in post_process (#16262) chore: Improves the flow to create a new chart (#16252) Move pagination outside of table (#16232)
craig-rueda
left a comment
craig-rueda
left a comment
There was a problem hiding this comment.
Left some comments. Please add some tests :).
| This task periodically hits charts to warm up the cache. | ||
|
|
||
| """ | ||
| logger.info("Loading strategy") |
There was a problem hiding this comment.
This should be DEBUG and should have a little more context - like Running cache warmup strategy x...
| """ | ||
| logger.info("Loading strategy") | ||
| class_ = None | ||
| extra_strategies: List[Type[Strategy]] = app.config["EXTRA_CACHING_STRATEGIES"] |
There was a problem hiding this comment.
Instead of EXTRA_CACHING_STRATEGIES, we should have CACHING_STRATEGIES, which is set to a default set of strategies. This way, downstreams can override the complete set of strategies easily.
| if class_.__name__ == strategy_name: | ||
| break | ||
| else: | ||
| message = f"No strategy {strategy_name} found!" |
There was a problem hiding this comment.
No f-strings in log messages.
Nit: Cache warmup strategy xxx not found
| logger.error(message, exc_info=True) | ||
| return message | ||
|
|
||
| logger.info("Loading %s", class_.__name__) |
| except TypeError: | ||
| message = "Error loading strategy!" | ||
| logger.exception(message) | ||
| return message |
| for url in strategy.get_urls(): | ||
| try: | ||
| logger.info("Fetching %s", url) | ||
| request.urlopen(url) # pylint: disable=consider-using-with |
There was a problem hiding this comment.
How does this deal with auth? I'm assuming most of these charts will live behind some auth mechanism?
dpgaspar
left a comment
dpgaspar
left a comment
There was a problem hiding this comment.
Left a question related with the scoped session, and adding tests would be great. @craig-rueda question regarding auth bugs me also
|
|
||
| def get_urls(self) -> List[str]: | ||
| urls = [] | ||
| session = db.create_scoped_session() |
There was a problem hiding this comment.
why? we should be using with session_scope(nullpool=True) as session
|
hi @amitmiran137 I see this was closed without merge, what was the reason?
so should we just invoke headless driver like we do for reports? |
4 participants
SUMMARY
Superset enables us to run different caching strategies under celery beat.
The ability here is to provide additional strategies from the config that can later be run if the strategy is chosen
In order to use Strategy type like
EXTRA_CACHING_STRATEGIES: List[Type[Strategy]] = []It was required to split from the single cache.py file into smaller modules in order to avoid importing superset.app
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION