gabotorresruiz commented on code in PR #45288:
URL: https://github.com/apache/superset/pull/45288#discussion_r4287098170
##########
superset/datasource/api.py:
##########
@@ -672,7 +678,14 @@ def compatible(self, datasource_type: str, datasource_id:
int) -> FlaskResponse:
).hexdigest()
)
- if (cached := cache_manager.data_cache.get(cache_key)) is not None:
+ timeout: int = datasource.cache_timeout or app.config.get(
Review Comment:
Not a blocker, more about the operator burden the docs now ask for. The
`compatible:` key material just above does not include `changed_on`, unlike
`_column_values_cache_key`, so a pre-existing no-expiry `compatible:` entry
stays reachable after re-enabling even once the datasource row is edited, while
the `col_values:` one falls out of its key on that same edit. I confirmed both
on a real Redis. That makes the manual invalidation mandatory for `compatible:`
and belt-and-braces for `col_values:`. Would adding `changed_on` here be in
scope, or better as a follow-up?
##########
docs/admin_docs/configuration/cache.mdx:
##########
@@ -112,6 +112,19 @@ value defined in `DATA_CACHE_CONFIG`.
Note, that by setting the cache timeout to `-1`, caching for charting data can
be disabled, either
per chart, dataset or database, or by default if set in `DATA_CACHE_CONFIG`.
+The datasource column-values and compatibility endpoints skip cache reads and
writes when
+their effective timeout is `-1`. They use the datasource timeout (for SQL
datasets, falling
+back to the database timeout), then the **top-level** `CACHE_DEFAULT_TIMEOUT`
when that value
+is unset or zero. They do not use `DATA_CACHE_CONFIG["CACHE_DEFAULT_TIMEOUT"]`
for this
+fallback, so setting only that nested default to `-1` does not disable these
endpoints.
Review Comment:
Not a blocker, but I think this paragraph describes a divergence that is not
actually there. The chart-data write path resolves the same fallback:
`set_and_log_cache` substitutes `app.config["CACHE_DEFAULT_TIMEOUT"]` whenever
the resolved timeout is `None`, and returns early on `-1`
(`superset/utils/cache.py:76-84`), so
`DATA_CACHE_CONFIG["CACHE_DEFAULT_TIMEOUT"]` is not the disable lever there
either. With the nested default at `-1` against a real Redis the chart-data
write still landed with the top-level TTL rather than being skipped. If that is
right, these endpoints already match chart data and the alignment follow-up may
be unnecessary.
Separately, with `-1` set only at the top level,
`QueryContext.get_cache_timeout()` returns `None`
(`superset/common/query_context.py:151-155`), so `get_df_payload_result` never
sets `force_query` and chart data keeps serving existing entries while skipping
writes. That is the same read-side half you are fixing here. Worth a follow-up,
or am I misreading that path?
##########
tests/unit_tests/semantic_layers/values_endpoint_test.py:
##########
@@ -577,3 +577,139 @@ def
test_unversioned_suggestion_cache_material_is_unchanged(
}
keys.append(cache.get.call_args.args[0])
assert keys[0] == keys[1]
+
+
[email protected]("use_default", [False, True])
[email protected]("warm", [False, True])
[email protected]("force", [False, True])
[email protected]("search", [False, True])
+def test_disabled_values_cache_skips_reads_and_writes(
+ client: Any,
+ full_api_access: None,
+ semantic_view_datasource: SemanticView,
+ mocker: MockerFixture,
+ use_default: bool,
+ warm: bool,
+ force: bool,
+ search: bool,
+) -> None:
+ """A disabled effective timeout bypasses warm and cold suggestion
caches."""
+ from flask import current_app
+
+ from superset.constants import CACHE_DISABLED_TIMEOUT
+
+ semantic_view_datasource.cache_timeout = (
+ None if use_default else CACHE_DISABLED_TIMEOUT
+ )
+ mocker.patch.dict(
+ current_app.config,
+ {"CACHE_DEFAULT_TIMEOUT": CACHE_DISABLED_TIMEOUT if use_default else
300},
+ )
+ cache: MagicMock =
mocker.patch("superset.datasource.api.cache_manager").data_cache
+ cache.get.return_value = ["stale"] if warm else None
+ response: Any = _get(
+ client,
+ f"category/values/?force={str(force).lower()}" + ("&q=oo" if search
else ""),
+ )
+
+ assert response.status_code == 200
+ assert response.json["result"] == ["Books", "Clothing"]
+ assert response.headers["X-Cache-Status"] == "MISS"
+ cache.get.assert_not_called()
+ cache.set.assert_not_called()
+ implementation: MagicMock = cast(MagicMock,
semantic_view_datasource.implementation)
+ implementation.get_values.assert_called_once()
+
+
[email protected]("use_default", [False, True])
[email protected]("warm", [False, True])
+def test_disabled_compatible_cache_skips_reads_and_writes(
+ client: Any,
+ full_api_access: None,
+ semantic_view_datasource: SemanticView,
+ mocker: MockerFixture,
+ use_default: bool,
+ warm: bool,
+) -> None:
+ """A disabled compatibility cache must recompute even with a stored
result."""
+ from flask import current_app
+
+ from superset.constants import CACHE_DISABLED_TIMEOUT
+
+ semantic_view_datasource.cache_timeout = (
+ None if use_default else CACHE_DISABLED_TIMEOUT
+ )
+ mocker.patch.dict(
+ current_app.config,
+ {"CACHE_DEFAULT_TIMEOUT": CACHE_DISABLED_TIMEOUT if use_default else
300},
+ )
+ metrics: MagicMock = mocker.patch.object(
+ SemanticView, "get_compatible_metrics", return_value=["revenue"]
+ )
+ dimensions: MagicMock = mocker.patch.object(
+ SemanticView, "get_compatible_dimensions", return_value=["category"]
+ )
+ cache: MagicMock =
mocker.patch("superset.datasource.api.cache_manager").data_cache
+ cache.get.return_value = {"stale": True} if warm else None
+
+ response: Any = client.post(
+ "/api/v1/datasource/semantic_view/1/compatible", json={}
+ )
+
+ assert response.status_code == 200
+ assert response.json["result"] == {
+ "compatible_metrics": ["revenue"],
+ "compatible_dimensions": ["category"],
+ }
+ metrics.assert_called_once_with([], [])
+ dimensions.assert_called_once_with([], [])
+ cache.get.assert_not_called()
+ cache.set.assert_not_called()
+
+
[email protected]("compatible", [False, True])
[email protected](
+ "datasource_timeout, default_timeout, expected_timeout",
+ [(120, 300, 120), (None, 300, 300), (0, 300, 300), (120, -1, 120)],
Review Comment:
Just a small NIT: could we add a `(None, 0, 0)` row here? A guard written as
`timeout > 0` instead of `timeout != CACHE_DISABLED_TIMEOUT` passes all 86
tests in this module as it stands, and that variant silently stops caching when
`CACHE_DEFAULT_TIMEOUT` is `0`, which Redis reads as no expiry rather than no
caching. The extra row fails on that variant and passes as written.
```suggestion
[(120, 300, 120), (None, 300, 300), (0, 300, 300), (120, -1, 120),
(None, 0, 0)],
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
