mikebridge commented on PR #42760: URL: https://github.com/apache/superset/pull/42760#issuecomment-5376420316
For reviewers and early adopters — the data-systems review pass that produced the hardening commit also catalogued the limitations we're consciously shipping with, so they're documented here rather than discovered in production. None serve wrong data except the second, which is an opt-in contract hazard; all are queued as follow-up work. **Known limitations accepted for this PR** - **Sentinel deployments are disabled, not fixed** (`initialize_semantic_cache`): the `UNSUPPORTED_BACKEND` gate is deliberate — flask-caching's `RedisSentinelCache` reads via `slave_for` replicas while writing the master, so the lease-guarded bucket read-modify-write loses read-your-writes under replication lag (a fresh descriptor can be invisible to the next lease holder, which then persists the stale bucket over it; the prune path's existence recheck has the same exposure). Follow-up: route repository reads through the write client, then drop the gate. - **Capability flags assume binary collation** (`cache_transform.py`, `SemanticCacheCapabilities`): leftover filters re-apply in pandas with byte-equality, case-sensitive semantics. A provider on a case-insensitive collation (MySQL `utf8mb4_ci`, SQL Server defaults) that sets `comparisons=True` or `pattern_escape` would serve different rows from cache than from the warehouse. Until the flag docs make this contract explicit, providers on case-insensitive collations should not opt in. - **Lease writes are unfenced** (`cache_coordination.py`): a holder paused past `lease_seconds` can overwrite its successor's bucket on resume; ownership loss is detected after the mutation. Bounded to lost descriptors and orphaned values until TTL — content-addressed value keys mean no interleaving can bind a descriptor to the wrong query's result — so this is an accepted efficiency anomaly, not a correctness one. Compare-and-set bucket writes would close it. - **No payload size cap**: full pyarrow results are pickled into the data cache; very large results should probably skip containment rather than flood the backend. Related: embedded guest tokens are folded whole into the scope identity, so every token (fresh `exp`/`iat`) mints a new bucket — near-zero reuse for embedded traffic. - **`cache_timeout=0` means never-expire** in cachelib, and identity rotation via `changed_on` strands the previous bucket with TTL as the only GC — operators using 0 should know those orphans are permanent. - **Smaller items**: fixed 50ms lease-acquisition spin without jitter (first-load herd on a hot view); descriptor LRU ordered by per-worker wall clocks (skew can misorder eviction); a release failure in the mutation's `finally` can mask the original exception in logs; async/Celery contexts bypass EXECUTION_CONTEXT containment by design (no request context — consistent misses, never wrong data); the `semantic_cache_hit` flag reflects the main query only, not time-offset sub-queries. The same pass explicitly verified the core safety properties: content-addressed value keys make the descriptor↔value binding race-proof and prevent cross-user/RLS reuse, provider execution never runs under the lease, release/refresh use compare-Lua (an expired owner cannot release its successor — integration-tested), and corrupted buckets self-heal. *Review and write-up produced with AI assistance (Claude); findings verified against the code and installed library behavior as described.* -- 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]
