michael-s-molina commented on a change in pull request #17882:
URL: https://github.com/apache/superset/pull/17882#discussion_r786714616
##########
File path: superset/config.py
##########
@@ -577,6 +577,15 @@ def _try_json_readsha(filepath: str, length: int) ->
Optional[str]:
"REFRESH_TIMEOUT_ON_RETRIEVAL": True,
}
+# Cache for chart form data
+CHART_FORM_DATA_CACHE_CONFIG: CacheConfig = {
+ "CACHE_TYPE": "filesystem",
Review comment:
Hi @ktmud. These are all great questions that also appeared while we
were discussing this feature internally. This is a great opportunity to post
the conclusions of those discussions 😄
> 1. We already have a short URL generator that produces a numeric ID. Will
this replace it?
We considered using this generator but the problem was its numerical
characteristics. It allows a user to guess potential IDs and it's not ideal for
the type of information that could be stored. The idea is to replace the short
URL generator with this new approach while maintaining backward compatibility.
This will be handled by another PR that is already in development.
> 2. Since the key is after a chart id, does this mean when users create a
new chart, we'd have to create a new chart object first, too? IIRC, currently
we don't do that.
We won't need to create a new chart object first. This part in the PR's
description talks about it:
> All the endpoints accept an optional query parameter called dataset_id
that is used for the cases when a chart is not saved yet but we want to
preserve the state. In this case, the chart ID shall be set to 0:
>
> `POST api/v1/chart/0/form_data?dataset_id=<id>`
In this case, the primary resource is the dataset and all its security rules
are applied.
> 3. We already have a
[KeyValue](https://github.com/apache/superset/blob/d2d4f8eb447c40a76e619d4cd6dec5ba21a9ea27/superset/models/core.py#L84-L90)
model that is more persistent than `Flask-Caching`. Is there a reason why we
are not reusing it? It feels to me both dashboard filter states and explore
chart states should be persistent since they will produce sharable URLs. We may
need a mechanism to periodically delete old records to reduce database size,
but that's better than cache server failure causing a URL to go stale at an
unpredictable time.
That was in fact our first approach. We discarded this solution because:
- The numerical nature of the IDs
- The lack of management properties associated with the entries prevents the
maintainability of the records.
- This feature is behind a feature flag and is disabled by default. Changing
its state constitutes a breaking change.
- We would need to develop the management system (we even did that during
the POC). This logic is already handled by Flask-Caching.
- We wanted to provide administrators with the possibility of choosing which
persistence mechanism is best suited for their use cases. Some will use Redis
as the main persistence mechanism while periodically [storing the memory
content on the disk](https://redis.io/topics/persistence). This can ensure
performance and persistence at the same time. Others will use the file system
to store this type of information and so on.
--
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
