mikebridge commented on PR #41940: URL: https://github.com/apache/superset/pull/41940#issuecomment-4938893725
## Review notes — multi-flush history fix (`bc0f897b`) _Agent-assisted review, read directly against the diff at head `bc0f897b`. Posting as notes on my own PR for the record._ ### Verdict: strong — the design is a real improvement, not just a patch Replacing "persist per-`after_flush` + dedup via `_PROCESSED_TXS_KEY`" (which fragilely dropped later-flush records) with **"capture each entity's earliest pre-state on every `before_flush`, then diff-and-persist once at `before_commit` after a forced final flush"** *structurally* eliminates the multi-flush failure mode, and does it in a net −81 lines. ### What's done well - **`_capture_dirty_entity_initial_state` skips already-seen keys** (`if key in initial_states: return`) → the earliest pre-state wins across flushes. Exactly right for the multi-flush case. - **Nested-transaction safety is correct** — `_reset_after_outer_transaction` only clears state when `transaction.parent is None` (outermost), so internal savepoints don't wipe accumulated state mid-transaction. - **Fail-open preserved** — `_build_scalar_buffer` wraps each entity's diff in `try/except → continue`; action-kind stamp, child-diff, meta-inject, and persist each swallow internally (persist under a SAVEPOINT so a caught error can't poison the commit on PostgreSQL). - **Excellent test coverage** — unit + integration tests cover exactly the tricky surface: multi-flush net-diff, nested **commit *and* rollback** preserving outer initial state, return-to-initial producing no spurious change, the `before_commit` force-flush re-entry guard, and terminal (commit/rollback) cleanup. - Module docstring + comments updated to the new model — no doc/code drift. ### Minor findings (none blocking) **1. Small fail-open gap in `finalize_change_records`.** The post-flush block is under `try/finally` (the `finally` only pops `_FINALIZING_KEY`) with no catch-all. Every record-writer helper self-guards, but `_current_transaction_id(session)` is called un-wrapped — if it ever raised (Continuum internal state), it would propagate out of `before_commit` and fail the user's commit. Not a regression, but wrapping the whole post-flush versioning block in one broad `except → log & return` would make the fail-open guarantee airtight. Worth doing since it's cheap. **2. `tx_id is None` with a non-empty buffer silently drops records** (no log). Correct to drop (nothing to attach them to), but a `logger.debug` there would help diagnose the anomaly if it ever occurs. **3. `before_commit` now forces a flush on _every_ commit app-wide.** Confirmed near-free (an empty flush short-circuits in SQLAlchemy without even firing `before_flush` — covered by `test_before_commit_with_no_work_does_not_flush`; versioned commits would flush at commit anyway), but it does shift flush *timing* slightly earlier globally when capture is enabled. A one-line comment noting that would save a future reader a double-take. Zero impact while the feature is flagged off. **4. Deletes (`session.deleted`) aren't diffed** (only `session.dirty`) — consistent with the existing design (delete → `operation_type=2` shadow, no field-level records); the docstring calls out `session.new` but not `session.deleted`. Worth a word for completeness. Net: correct, simpler than what it replaces, and unusually well-tested for a flush/commit-lifecycle change. I'd fold in #1; #2–#4 are optional polish. -- 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]
