potiuk commented on PR #70839:
URL: https://github.com/apache/airflow/pull/70839#issuecomment-5149352599
Sensible thing to make configurable — a hardcoded 30 second auth-decision
cache is exactly the sort of thing deployments need to tune. Two things before
merge.
**`version_added` needs a real version.** Every other option in
`provider.yaml` carries one (`0.0.1`, `0.4.0`), and the equivalent FAB option
uses `version_added: 3.2.0`. The provider is currently at `0.8.2`, so this
wants `0.9.0`:
```yaml
cache_ttl_seconds:
description: |
Time (in seconds) to cache auth decisions in ``bulk
is_authorized_{resource}`` methods.
type: integer
version_added: 0.9.0
example: ~
default: "30"
```
**The import-time read is what forces `reload()` in the tests, and that's
worth avoiding.**
```python
_CACHE_TTL_SECONDS = conf.getint(CONF_SECTION_NAME,
CONF_CACHE_TTL_SECONDS_KEY, fallback=30)
```
Because this is evaluated once at import, the tests need an autouse fixture
that reloads the module to change it. Reloading mid-suite is fragile — it
rebinds `_cache` and `_pending_requests` to fresh dicts while the
`_clear_cache` fixture is holding references to the previous ones, so the two
fixtures are operating on different objects depending on ordering. The autouse
fixture also pins the TTL to 60 for every pre-existing test in the file,
quietly changing their baseline.
I know `FabAuthManager` does read its `cache_ttl` at import, so there is
precedent. But it's precedent worth breaking rather than following, because FAB
shows the cost: its test decorates with
```python
@conf_vars({("fab", "cache_ttl"): "1"})
def test_deserialize_user(...):
```
and that has no effect whatsoever — `CACHE_TTL` was already baked into
`TTLCache(maxsize=1024, ttl=CACHE_TTL)` when the class body was evaluated at
import. The test passes because caching happens at all, not because the
configured TTL was honoured. You clearly spotted this hazard, which is why the
reload is there at all.
Reading it lazily makes the whole problem go away:
```python
def _cache_ttl_seconds() -> int:
return conf.getint(CONF_SECTION_NAME, CONF_CACHE_TTL_SECONDS_KEY,
fallback=30)
```
with the two call sites using `_cache_ttl_seconds()` instead of the
constant. Then `conf_vars` works normally, the reload fixture disappears,
`test_configure_ttl` becomes a plain assertion, and — the part that matters for
operators — the setting can be changed without restarting the API server, which
is most of the reason to make it configurable in the first place.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
--
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]
