feat(web-analytics): add dedicated eager backfill job with 24h budget - #70592
Conversation
|
Reviews (1): Last reviewed commit: "feat(web-analytics): add dedicated eager..." | 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: 8c70ed66af
ℹ️ 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".
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Manual-only Dagster backfill job reusing a well-tested op; the two substantive greptile/codex concerns (global setting leak, unresumable audience across chunks, run overlap) were each fixed in the current head with a try/finally restore, a job-level concurrency guard, and a widened lookback window, and greptile reacted 👍 to the fix replies. Author owns the file and has STRONG familiarity (100% of touched lines, 38 merged PRs in this path), which covers assurance for this non-schema, non-API, non-auth internal tooling change.
- Author wrote 100% of the modified lines and has 38 merged PRs in these paths (familiarity STRONG).
- 👍 on the PR from greptile-apps[bot], hex-security-app[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 348L, 2F substantive — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (348L, 2F, two-areas, feat) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ ce5a82c · reviewed head a20353f |
🤖 CI report
|
New commits pushed (delta classified non_trivial_delta) — stamphog approval dismissed; re-review running automatically.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| older = [ | ||
| r | ||
| for r in in_flight | ||
| if r.dagster_run.run_id != context.run_id and (ours is None or r.create_timestamp <= ours.create_timestamp) | ||
| ] |
There was a problem hiding this comment.
Race condition in concurrent run detection. If two runs are launched simultaneously and receive identical create_timestamp values, both will see each other as "older" (due to the <= comparison) and both will fail, preventing any backfill from proceeding.
# Should use strict < instead of <=
older = [
r
for r in in_flight
if r.dagster_run.run_id != context.run_id and (ours is None or r.create_timestamp < ours.create_timestamp)
]With strict <, equal timestamps mean neither run is strictly older, so only genuinely older runs will trigger the failure.
| older = [ | |
| r | |
| for r in in_flight | |
| if r.dagster_run.run_id != context.run_id and (ours is None or r.create_timestamp <= ours.create_timestamp) | |
| ] | |
| older = [ | |
| r | |
| for r in in_flight | |
| if r.dagster_run.run_id != context.run_id and (ours is None or r.create_timestamp < ours.create_timestamp) | |
| ] | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Contained addition of a manual-only backfill Dagster job reusing the existing warm op; the flagged greptile/codex concerns (global-setting leak, concurrent-run overlap, audience drift across chunks) were all fixed and marked resolved with bot thumbs-up, and the reported trunk-io test failures match the job_name access pattern this diff's try/except already guards against. Author is on the owning team with STRONG familiarity.
- Author wrote 100% of the modified lines and has 38 merged PRs in these paths (familiarity STRONG).
- 👍 on the PR from greptile-apps[bot], hex-security-app[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 354L, 2F substantive — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (354L, 2F, two-areas, feat) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ f93c4d0 · reviewed head 322b308 |
1 participant
Problem
#70544 added
active_teams_pctto the eager warmer's run config, but the warm job's 90-minutemax_runtimeis sized for hourly maintenance, not a fleet backfill: the op warms teams through a fixed 10-worker pool, so a bigger audience doesn't raise instantaneous cluster load - it only runs longer. A 100% backfill (~9k active teams, ~1.2M window inserts at the measured ~27k/h ceiling) needs ~40+ hours, which the maintenance job can only grind through in truncated 90-minute slices.Changes
New manual-only Dagster job
web_analytics_eager_backfill, reusing the existing warm op unchanged:active_teams_pct: 100- launch with no config for a full-fleet backfill, or override the pct for a partial one.dagster/max_runtimeof 24h instead of 90min. Fully resumable: windows built before a termination persist via their TTLs, so a relaunch skips them and continues the tail.How did you test this code?
Automatic notifications
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Authored with Claude Code. Lucas observed the fixed-concurrency property during the live 5% ramp (bigger audience = longer runtime, not more pressure) and asked for a backfill-specific job rather than raising the schedule's percentage. Originally committed to the #70544 branch, which had already merged - cherry-picked to this fresh branch.