rusackas commented on PR #40221: URL: https://github.com/apache/superset/pull/40221#issuecomment-4488505624
Dropping some "findings" here in case they're helpful: 1. Thread safety / race condition in store_result The store operation involves: (1) read the index, (2) append the new entry, (3) write the index back. This is a read-modify-write on a Redis key that is not atomic. Under concurrent writes for the same view, two workers could read the same index simultaneously, each append their entry, and one write would overwrite the other's addition. A Redis WATCH/MULTI/EXEC transaction or a Lua script would eliminate this race. Under normal Superset load with one worker per view, this is unlikely but not impossible — especially in a heavily concurrent dashboard load. 2. Test coverage is critically low: 19.93% patch coverage 3. The field import in from dataclasses import dataclass, field (line 44) — field doesn't appear to be used in CachedEntry. If unused, it should be removed. 4. The _sql_like_to_regex function is listed in the symbol table — this is a non-trivial implementation. LIKE pattern → regex conversion has many edge cases (%, _, escaping). Make sure the test suite covers escaped wildcard characters (e.g., LIKE '100\%' 5. ROLLUP correctness: guarding against stale cache entries When the cache tries to serve a ROLLUP result, there are two reasons it might skip a candidate and move to the next one: * The value was evicted from Redis — Redis may have deleted the cached data (due to TTL or memory pressure) between when the code found it in the index and when it tried to fetch it. If that happens, the code correctly skips it. * The cached result is incomplete — If a cached query had a row limit and that limit was fully reached, we can't safely roll up those rows to answer a different grouping (because we're only seeing the "top N" rows of the original query, not the full dataset). So the code also skips those. The concern is that these two checks happen in sequence, and the interaction between them should be well-tested. Specifically: what if an entry is evicted and would have been rejected as incomplete anyway? What if the only ROLLUP candidate gets evicted and there's no fallback? The good news is that `test_serve_falls_back_to_rollup_when_exact_value_evicted` in the integration test does cover the eviction fallback path, which addresses the main worry. This is more of a "make sure the test coverage keeps up with the edge cases" note than a definitive bug. -- 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]
