NoiceHax commented on PR #43149: URL: https://github.com/apache/superset/pull/43149#issuecomment-5296781085
Applied the review point about the config lookup, but not exactly as suggested. Moving the `cache_timeout` pop itself below the `should_cache` early return breaks the uncached path. Callers like `DatabaseTablesCommand` pass `cache=` and `cache_timeout=` together, and the decorated functions (`get_all_table_names_in_schema` etc) do not take a `cache_timeout` argument. If the pop happens after the early return, `cache_timeout` stays in kwargs and gets forwarded, so `cache=False` raises TypeError: got an unexpected keyword argument 'cache_timeout'. So I kept the pop where it was and moved only the `app.config["CACHE_DEFAULT_TIMEOUT"]` lookup below the return. That gets the actual benefit (skipping the cache no longer touches the app config) with no behaviour change. Added a test covering `cache=False, cache_timeout=None` that asserts the config is never read and the wrapped function still runs. On CI, the only red job is test-mysql, and it is an unrelated flake: `test_get_saved_query` asserting `changed_on_delta_humanized == 'now'` got 'a second ago'. Timestamp boundary race in the saved query fixture, nothing to do with this change. test-postgres, test-sqlite and unit-tests all passed in the same run. -- 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]
