feat: implement cache invalidation api #10761

bkyryliuk · 2020-09-02T05:04:57Z

SUMMARY

Implements a way to invalidate cached data for the provide datasources.

TEST PLAN

unit tests
local testing

ADDITIONAL INFORMATION

superset/cache/api.py

dpgaspar · 2020-09-02T15:17:01Z

superset/cache/api.py

+            return self.response_400(message="Request is incorrect")
+        except ValidationError as error:
+            return self.response_400(
+                message=_("Request is incorrect: %(error)s", error=error.messages)


nit: Just use: return self.response_400(message=error.messages)

codecov-commenter · 2020-09-03T04:57:24Z

Codecov Report

Merging #10761 into master will decrease coverage by 0.32%.
The diff coverage is 96.77%.

@@            Coverage Diff             @@
##           master   #10761      +/-   ##
==========================================
- Coverage   61.32%   60.99%   -0.33%     
==========================================
  Files         803      806       +3     
  Lines       37927    38002      +75     
  Branches     3561     3561              
==========================================
- Hits        23258    23180      -78     
- Misses      14483    14636     +153     
  Partials      186      186

Flag	Coverage Δ
#javascript	`61.59% <ø> (+<0.01%)`	⬆️
#python	`60.65% <96.77%> (-0.52%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/cachekeys/api.py	`95.91% <95.91%> (ø)`
superset/app.py	`80.30% <100.00%> (+0.15%)`	⬆️
superset/cachekeys/schemas.py	`100.00% <100.00%> (ø)`
superset/charts/schemas.py	`100.00% <100.00%> (ø)`
superset/db_engines/hive.py	`0.00% <0.00%> (-85.72%)`	⬇️
superset/db_engine_specs/hive.py	`53.90% <0.00%> (-30.08%)`	⬇️
superset/db_engine_specs/presto.py	`70.85% <0.00%> (-10.77%)`	⬇️
superset/connectors/base/views.py	`71.42% <0.00%> (-3.58%)`	⬇️
superset/examples/world_bank.py	`97.10% <0.00%> (-2.90%)`	⬇️
superset/examples/birth_names.py	`97.36% <0.00%> (-2.64%)`	⬇️
... and 15 more

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 8a9ae81...115de31. Read the comment docs.

bkyryliuk · 2020-09-04T17:24:09Z

tests/superset_test_config.py

@@ -76,6 +76,14 @@ def GET_FEATURE_FLAGS_FUNC(ff):
 REDIS_PORT = os.environ.get("REDIS_PORT", "6379")
 REDIS_CELERY_DB = os.environ.get("REDIS_CELERY_DB", 2)
 REDIS_RESULTS_DB = os.environ.get("REDIS_RESULTS_DB", 3)
+REDIS_CACHE_DB = os.environ.get("REDIS_CACHE_DB", 4)
+
+CACHE_CONFIG = {


needed for cache to work on CI

bkyryliuk · 2020-09-04T17:24:48Z

@willbarrett , @dpgaspar - ready for review

mistercrunch · 2020-09-06T00:09:07Z

superset/cachekeys/api.py

@@ -0,0 +1,98 @@
+# Licensed to the Apache Software Foundation (ASF) under one


I think the route /api/v1/cachekey/invalidate is not ideal. I'm struggling to find something good though

agree, I had the same struggles, definitely open to suggestions.
/api/v1/cache/invalidate could be slightly better

How about DELETE /api/v1/cachekey/<cachekey>?

it would be too much struggle to fetch cache keys though the API and then issue deletes for those.
I think we could expose GET / DELETE later once those use cases will arise

mistercrunch · 2020-09-06T00:10:06Z

superset/cachekeys/api.py

+    @statsd_metrics
+    def invalidate(self) -> Response:
+        """
+        Takes a list of datasources, finds the associated cache records and


Side note (outside the scope of this PR), we'll be renaming datasource to dataset. I think it's mostly done in the UI layer, but has yet to be done in the code.

good catch, I can do it in the separate PR if that works for you, would rename it in API & CacheKey model for consistency.

dpgaspar

Left a couple of comments

dpgaspar · 2020-09-10T15:56:10Z

superset/cachekeys/api.py

+
+class CacheRestApi(BaseSupersetModelRestApi):
+    datamodel = SQLAInterface(CacheKey)
+    resource_name = "cache"


nit: since this was renamed to cache, evaluate if placing everything on cachekeys still makes sense

renamed to cachekey, I am happy with either way naming it cachekey or cache
I was thinking about moving it into separate class from the cache key, but most of the instrumentation lives in BaseSupersetModelRestApi that requires to model.

dpgaspar · 2020-09-10T16:07:36Z

superset/cachekeys/api.py

+            .all()
+        )
+        if cache_keys:
+            cache_manager.cache.delete_many(*[c.cache_key for c in cache_keys])


A couple of questions:

if a cache delete_many fails, we may end up with delete keys on the cache that still exist on CacheKey will a next run of delete_many fail if it can't find a key?

the same applies if a delete CacheKey fails, would also be safer to try/except and rollback

delete_many doesn't fail, only returns true / false if the keys were deleted. e.g. if the key was not found it will return false. Updated CacheKey logic to delete is a single statement & rollback if fails -> that would be more robust.

deleting case & keeping records is not too bad as there still a need to garbage collect those and cache is not a persistent store.

superset/cachekeys/schemas.py

dpgaspar · 2020-09-10T16:18:57Z

tests/base_tests.py

@@ -69,6 +71,39 @@ def get_resp(
    return resp.data.decode("utf-8")


+def post_assert_metric(


Is this the same but on a different place?

yes same code, moved it into this function for reuse in the pytests

tests/cachekeys/api_tests.py

dpgaspar

Do you want all the ModelRestApi route methods to be available? If not include include_route_methods = {"invalidate",} also note this will create a new specific permission can invalidate on Cache key.
Check https://localhost:8080/swagger/v1

bkyryliuk · 2020-09-14T17:52:04Z

Do you want all the ModelRestApi route methods to be available? If not include include_route_methods = {"invalidate",} also note this will create a new specific permission can invalidate on Cache key.
Check https://localhost:8080/swagger/v1

addressed, thanks

bkyryliuk · 2020-09-14T17:56:02Z

* Add cache endpoints * Implement cache endpoint * Tests and address feedback * Set cache config * Address feedback * Expose only invalidate endpoint Co-authored-by: bogdan kyryliuk <bogdankyryliuk@dropbox.com>

benceorlai · 2020-11-30T01:59:21Z

Roadmap: apache-superset/superset-roadmap#74

ktmud · 2021-01-23T09:20:34Z

superset/cachekeys/api.py

+
+    @expose("/invalidate", methods=["POST"])
+    @event_logger.log_this
+    @protect()


It seems this API is not actually protected---it's probably missing a @permission_name("edit"). Anyone can trigger cache key invalidation with a cURL:

curl -X POST --data '{"datasource_uids": ["3__table"]}' --header "Content-Type: application/json" "http://localhost:8088/api/v1/cachekey/invalidate"

Is this by design?

cc @bkyryliuk

pull-request-size bot added the size/L label Sep 2, 2020

bkyryliuk marked this pull request as draft September 2, 2020 05:05

dpgaspar reviewed Sep 2, 2020

View reviewed changes

superset/cache/api.py Outdated Show resolved Hide resolved

dpgaspar reviewed Sep 2, 2020

View reviewed changes

bkyryliuk force-pushed the bogdan/cache_endpoints branch 2 times, most recently from 210cffd to 2028348 Compare September 3, 2020 04:49

bkyryliuk force-pushed the bogdan/cache_endpoints branch 9 times, most recently from 942ede8 to f119c85 Compare September 4, 2020 16:33

bkyryliuk commented Sep 4, 2020

View reviewed changes

bkyryliuk requested a review from willbarrett September 4, 2020 17:24

bkyryliuk marked this pull request as ready for review September 4, 2020 17:24

bkyryliuk changed the title ~~feat: [WIP] implement cache invalidation api~~ feat: implement cache invalidation api Sep 4, 2020

mistercrunch reviewed Sep 6, 2020

View reviewed changes

bkyryliuk force-pushed the bogdan/cache_endpoints branch 2 times, most recently from 4bfc2d3 to 49e0eea Compare September 10, 2020 15:07

dpgaspar reviewed Sep 10, 2020

View reviewed changes

bkyryliuk force-pushed the bogdan/cache_endpoints branch 2 times, most recently from 115de31 to fc4f9d2 Compare September 11, 2020 16:08

bogdan-dbx added 2 commits September 11, 2020 09:35

Add cache endpoints

49f9ffb

Implement cache endpoint

9e90958

bogdan-dbx added 2 commits September 11, 2020 09:35

Tests and address feedback

2877170

Set cache config

90f2a1d

bkyryliuk force-pushed the bogdan/cache_endpoints branch 2 times, most recently from 682007a to ecb9672 Compare September 11, 2020 16:37

Address feedback

b87299f

bkyryliuk force-pushed the bogdan/cache_endpoints branch from ecb9672 to b87299f Compare September 11, 2020 23:39

dpgaspar requested changes Sep 14, 2020

View reviewed changes

Expose only invalidate endpoint

8d31609

dpgaspar approved these changes Sep 15, 2020

View reviewed changes

bkyryliuk merged commit 9c420d6 into apache:master Sep 15, 2020

ktmud reviewed Jan 23, 2021

View reviewed changes

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.38.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement cache invalidation api #10761

feat: implement cache invalidation api #10761

bkyryliuk commented Sep 2, 2020 •

edited

Loading

dpgaspar Sep 2, 2020

codecov-commenter commented Sep 3, 2020 •

edited

Loading

bkyryliuk Sep 4, 2020

bkyryliuk commented Sep 4, 2020

mistercrunch Sep 6, 2020

bkyryliuk Sep 10, 2020

willbarrett Sep 15, 2020

bkyryliuk Sep 15, 2020

mistercrunch Sep 6, 2020

bkyryliuk Sep 10, 2020

dpgaspar left a comment

dpgaspar Sep 10, 2020

bkyryliuk Sep 10, 2020

dpgaspar Sep 10, 2020

bkyryliuk Sep 11, 2020

dpgaspar Sep 10, 2020

bkyryliuk Sep 10, 2020

dpgaspar left a comment

bkyryliuk commented Sep 14, 2020

bkyryliuk commented Sep 14, 2020

benceorlai commented Nov 30, 2020

ktmud Jan 23, 2021 •

edited

Loading

		@@ -0,0 +1,98 @@
		# Licensed to the Apache Software Foundation (ASF) under one

		@@ -69,6 +71,39 @@ def get_resp(
		return resp.data.decode("utf-8")


		def post_assert_metric(

feat: implement cache invalidation api #10761

feat: implement cache invalidation api #10761

Conversation

bkyryliuk commented Sep 2, 2020 • edited Loading

SUMMARY

TEST PLAN

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

codecov-commenter commented Sep 3, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

bkyryliuk commented Sep 4, 2020

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

dpgaspar left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpgaspar left a comment

Choose a reason for hiding this comment

bkyryliuk commented Sep 14, 2020

bkyryliuk commented Sep 14, 2020

benceorlai commented Nov 30, 2020

ktmud Jan 23, 2021 • edited Loading

Choose a reason for hiding this comment

bkyryliuk commented Sep 2, 2020 •

edited

Loading

codecov-commenter commented Sep 3, 2020 •

edited

Loading

ktmud Jan 23, 2021 •

edited

Loading