ktmud commented on a change in pull request #19232: URL: https://github.com/apache/superset/pull/19232#discussion_r830356952
########## File path: superset/key_value/cache.py ########## @@ -0,0 +1,110 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from datetime import datetime, timedelta +from hashlib import md5 +from typing import Any, Dict, List, Optional +from uuid import UUID, uuid3 + +from flask import Flask +from flask_caching import BaseCache Review comment: Would it make more sense to put this file under `superset/extensions/key_value_cache.py`? The rational is `superset.key_value` as an entity model should not depend on a Flask extension. ########## File path: superset/key_value/cache.py ########## @@ -0,0 +1,110 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from datetime import datetime, timedelta +from hashlib import md5 +from typing import Any, Dict, List, Optional +from uuid import UUID, uuid3 + +from flask import Flask +from flask_caching import BaseCache + +from superset.key_value.exceptions import KeyValueCreateFailedError +from superset.key_value.types import KeyType + +RESOURCE = "superset_cache" +KEY_TYPE: KeyType = "uuid" Review comment: Linking my previous comment about KeyType in case you missed it: https://github.com/apache/superset/pull/19078#discussion_r828558897 ########## File path: superset/key_value/cache.py ########## @@ -0,0 +1,110 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from datetime import datetime, timedelta +from hashlib import md5 +from typing import Any, Dict, List, Optional +from uuid import UUID, uuid3 + +from flask import Flask +from flask_caching import BaseCache + +from superset.key_value.exceptions import KeyValueCreateFailedError +from superset.key_value.types import KeyType + +RESOURCE = "superset_cache" +KEY_TYPE: KeyType = "uuid" + + +class SupersetCache(BaseCache): + def __init__(self, namespace: UUID, default_timeout: int = 300) -> None: + super().__init__(default_timeout) + self.namespace = namespace + + @classmethod + def factory( + cls, app: Flask, config: Dict[str, Any], args: List[Any], kwargs: Dict[str, Any] + ) -> BaseCache: + # base namespace for generating deterministic UUIDs + md5_obj = md5() + seed = config.get("CACHE_KEY_PREFIX", "") + md5_obj.update(seed.encode("utf-8")) + kwargs["namespace"] = UUID(md5_obj.hexdigest()) + return cls(*args, **kwargs) + + def get_key(self, key: str) -> str: + return str(uuid3(self.namespace, key)) + + @staticmethod + def _prune() -> None: + # pylint: disable=import-outside-toplevel + from superset.key_value.commands.delete_expired import ( + DeleteExpiredKeyValueCommand, + ) + + DeleteExpiredKeyValueCommand(resource=RESOURCE).run() + + def get_expiry(self, timeout: Optional[int]) -> datetime: + return datetime.now() + timedelta(seconds=timeout or self.default_timeout) + + def set(self, key: str, value: Any, timeout: Optional[int] = None) -> bool: + # pylint: disable=import-outside-toplevel + from superset.key_value.commands.delete import DeleteKeyValueCommand + + DeleteKeyValueCommand( + resource=RESOURCE, key_type=KEY_TYPE, key=self.get_key(key), + ).run() Review comment: Instead of `delete` then `add` (two SQL queries), can we implemented a `UpdateKeyValueCommand` command instead? ########## File path: superset/utils/cache_manager.py ########## @@ -40,27 +38,27 @@ def _init_cache( ) -> None: cache_config = app.config[cache_config_key] cache_type = cache_config.get("CACHE_TYPE") - if app.debug and cache_type is None: - cache_threshold = cache_config.get("CACHE_THRESHOLD", math.inf) + if required and cache_type in (None, "SupersetCache"): + if cache_type is None: + logger.warning( + "Falling back to the built-in cache, that stores data in the " Review comment: Are we sure we want to show this warning when users specifically set cache type to `SupersetCache`? It's not "falling back" at least. ########## File path: superset/key_value/cache.py ########## @@ -0,0 +1,110 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from datetime import datetime, timedelta +from hashlib import md5 +from typing import Any, Dict, List, Optional +from uuid import UUID, uuid3 + +from flask import Flask +from flask_caching import BaseCache + +from superset.key_value.exceptions import KeyValueCreateFailedError +from superset.key_value.types import KeyType + +RESOURCE = "superset_cache" +KEY_TYPE: KeyType = "uuid" + + +class SupersetCache(BaseCache): + def __init__(self, namespace: UUID, default_timeout: int = 300) -> None: + super().__init__(default_timeout) + self.namespace = namespace + + @classmethod + def factory( + cls, app: Flask, config: Dict[str, Any], args: List[Any], kwargs: Dict[str, Any] + ) -> BaseCache: + # base namespace for generating deterministic UUIDs + md5_obj = md5() + seed = config.get("CACHE_KEY_PREFIX", "") + md5_obj.update(seed.encode("utf-8")) + kwargs["namespace"] = UUID(md5_obj.hexdigest()) + return cls(*args, **kwargs) + + def get_key(self, key: str) -> str: + return str(uuid3(self.namespace, key)) + + @staticmethod + def _prune() -> None: + # pylint: disable=import-outside-toplevel + from superset.key_value.commands.delete_expired import ( + DeleteExpiredKeyValueCommand, + ) + + DeleteExpiredKeyValueCommand(resource=RESOURCE).run() + + def get_expiry(self, timeout: Optional[int]) -> datetime: Review comment: ```suggestion def _get_expiry(self, timeout: Optional[int]) -> datetime: ``` Maybe this should be a private function? Flask-Caching's `SimpleCache` also called this function [BaseCache._normalize_timeout](https://github.com/pallets-eco/flask-caching/blob/e3440f3601061e3732f920a59f31d154c11d73ba/src/flask_caching/backends/simplecache.py#L71), maybe we should follow the same convention? (It also ignores negative timeout) ########## File path: superset/key_value/cache.py ########## @@ -0,0 +1,110 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from datetime import datetime, timedelta +from hashlib import md5 +from typing import Any, Dict, List, Optional +from uuid import UUID, uuid3 + +from flask import Flask +from flask_caching import BaseCache + +from superset.key_value.exceptions import KeyValueCreateFailedError +from superset.key_value.types import KeyType + +RESOURCE = "superset_cache" +KEY_TYPE: KeyType = "uuid" + + +class SupersetCache(BaseCache): Review comment: ```suggestion class SupersetMetastoreCache(BaseCache): ``` Not sure if it's worth the verboseness, but would `SupersetKeyValueCache` or `SupersetMetastoreCache` be a better name because `SupersetCache` sounds a little too generic? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
