feat(web-analytics): widen warming to min-2 shapes and parallelize the warm pass#73013
Conversation
|
😎 This pull request was merged. |
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
products/web_analytics/dags/cache_warming.py:356
**Worker Tags Leak Between Shapes**
Executor threads are reused, and `tag_queries()` updates the current tag set rather than replacing it. After one runner adds fields such as its cache key, query name, or execution mode, the next task on that thread retains those fields until it overwrites them, so setup queries for the next shape can be logged with metadata from the previous shape.
### Issue 2 of 3
products/web_analytics/dags/cache_warming.py:382
**Failure Logging Is Concurrent**
When several shapes fail together, multiple workers call the shared Dagster `context.log` concurrently. The sibling eager warmer serializes this logger because it is not guaranteed to be thread-safe; this path can therefore lose failure records or raise another logging error while handling the original exception.
### Issue 3 of 3
products/web_analytics/dags/cache_warming.py:369-378
**Duplicate Cache Keys Warm Concurrently**
The selection groups by raw JSON text, while the runner derives its cache key from parsed query data. If two differently encoded rows normalize to the same team and cache key, separate workers can both observe a missing or stale entry and replay the same query concurrently, wasting ClickHouse capacity and recording duplicate warmed outcomes.
Reviews (1): Last reviewed commit: "feat(web-analytics): widen warming to mi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 982c67364e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
🤖 CI report
|
d17f1a6 to
7062309
Compare
…er in workers, cache-key dedupe
7062309 to
5a14b48
Compare
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
This touches background query-warming compute (selection threshold, cache-key dedup, concurrency), but the author owns this code (STRONG familiarity, same-team), and every substantive reviewer concern (tag leak, concurrent logging, amplification risk, duplicate-key replay) is verifiably fixed in the diff and covered by new tests; the one remaining unresolved comment is a metrics double-counting edge case with no runtime or data-safety impact.
- Author wrote 84% of the modified lines and has 56 merged PRs in these paths (familiarity STRONG).
- 👍 on the PR from greptile-apps[bot], hex-security-app[bot].
- Unresolved @graphite-app comment: duplicate-cache-key check runs after the raw-low-demand check, so two duplicate raw-path low-demand shapes both increment skipped_raw_low_demand instead of one being labeled skipped_duplicate — a metrics-accuracy nit only (no query is replayed twice, no data impact), worth a quick follow-up fix.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 171L, 2F substantive, 304L/3F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (304L, 3F, two-areas, feat) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ cc5cfb1 · reviewed head 4b89f42 |
1 participant
Problem
Two follow-ups to the demand-driven warmer now that #72959 made misses harmless (live serve + warm-behind):
warm_queries_opprocesses shapes strictly sequentially. At the widened selection size a cold pass would take hours; the pass is IO-bound, so a small thread pool cuts wall time ~8x.Changes
WEB_ANALYTICS_WARMING_MIN_QUERY_COUNTdefault 10 → 2 andWEB_ANALYTICS_WARMING_MAX_SHAPES20000 → 40000 (sized above the ~29k the min-2 selection produces so the cap doesn't silently truncate). Code defaults, not instance-setting flips — both remain tunable via instance settings.warm_queries_opruns shapes through aThreadPoolExecutor(8 workers, mirroring the eager warmer's pool pattern): teams prefetched in one query, per-shape outcomes aggregated from the pool,close_old_connections()per task.sync_execute called with missing query tagswarnings.How did you test this code?
pytest products/web_analytics/dags/tests/test_cache_warming.py(28 passed). New test: worker threads observeis_background_warming_request() == True— catches the thread-local-tags regression above, which no existing test covered (the old sequential op tagged in the op thread and happened to work).Automatic notifications
Docs update
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Claude Code session; Lucas asked for the min-2 default (in code, not instance-setting flips) and the warm-pass parallelization after #72959 removed the inline-build risk. Concurrency of 8 chosen to stay well under the OFFLINE per-user query-slot budget; the eager warmer's ThreadPoolExecutor pattern (contained worker exceptions, connection hygiene) was reused, with the thread-local tagging bug it exhibits fixed here rather than inherited. Commit is unsigned: the GPG signing agent was unavailable in this session.