mikebridge commented on PR #43490: URL: https://github.com/apache/superset/pull/43490#issuecomment-5535100528
@aminghadersohi — thanks, Finding 1 is a fair call. Addressed at `e2dddd9da7`. **On the lock scope itself — kept deliberately, not narrowed.** You praised the lock-then-clock ordering, and that's exactly why I didn't narrow it: holding the lock across the DELETE is what serializes a pruning batch with `write_ahead`'s row so a concurrent write commits only *after* the batch (invisible to its SELECT). A "timestamp-only prelude" narrowing would reintroduce the race, and I can't prove a safe narrowing correct without concurrency test coverage the environment here can't run. So per the tradeoff you flagged, the window is bounded instead — `BATCH_SIZE=500` caps rows per statement and the pruning indexes back the boundary/per-entity subqueries — and the liveness cost is documented rather than papered over. **What changed (docs only, no logic):** - `write_ahead`: added the load-bearing comment you asked for on why the lock precedes `created_on=utc_now()`. - `_delete_batch`: comment on why the lock spans the DELETE + the bounded liveness tradeoff. - `UPDATING.md`: the operator-facing note — a concurrent purge's write-ahead can block until the batch commits; it's fail-closed-and-retried where a lock/statement timeout applies, so a skipped cycle, not data loss. **Two accuracy corrections a self-review of these comments surfaced, worth flagging since they touch your finding:** 1. The guarantee is transaction **visibility** (isolation), not a wall-clock one. A pure `created_on > now` claim only holds under bounded clock skew (`write_ahead` and the pruner are separate tasks with independent clocks — and the module already guards writer skew via the future-row exclusion in `_streak_boundary_subquery`). The comments now lead with visibility and name the streak invariants, not wall-clock ordering, as the primary safeguard. 2. "Fails closed on the lock-wait timeout" isn't true for default PostgreSQL (`lock_timeout` disabled → it waits then succeeds). The docs now spell out the dialect split (PG waits-then-succeeds; MySQL `innodb_lock_wait_timeout` / SQLite / a configured timeout fail closed). Full 4-lens pass (clean-code/tidy-first/python/sqlalchemy) on the change is filed in the spec repo. The rebase (single alembic head at `8f31c5d726ab`), the stale-approval note, and the earlier delta-review fix are all still in place. Would appreciate another look. -- 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]
