Backport #32953 to 2.0: stop a single unevaluable event from discarding its whole change-event batch - #33282
Conversation
…le change-event batch (#32953)
d1250ed to
24671f0
Compare
✅ Playwright Results — workflow succeededValidated commit ✅ 795 passed · ❌ 0 failed · 🟡 1 flaky · ⏭️ 0 skipped · 🧰 0 lifecycle flaky PerformanceBlocking targets: ✅ met · Optimization targets: 🟡 in progress Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting. 🕒 Full workflow signal wall (to summary) 50m 25s ⏱️ Max setup 2m 57s · max shard execution 18m 48s · max shard-job elapsed before upload 21m 44s · reporting 4s 🌐 206.53 requests/attempt · 2.62 app boots/UI scenario · 23.76% common-shard skew Optimization targets still in progress:
🟡 1 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
Code Review ✅ ApprovedBackport of #32953 to OptionsDisplay: compact → Counting what did not apply, without listing it. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
Backport of #32953 (Fixes #31331) to
2.0.Problem
Alert filters on owners or domains re-read the entity from the database. Some entity types, such as
useranddomain, do not have anownersordomainsfield, so that read throwsInvalid field name. The exception escapes the filter, and the consumer still moves its offset forward, so every other event in the same batch is silently dropped: no delivery, no retry, no failed-event record. While such an event is unprocessed,/diagnosticInfofor the alert also returns 500.#32953 fixes this on
mainin two ways: filters skip the read when the entity type cannot have the field, and an event whose filters still fail is excluded on its own instead of taking the batch down with it.Why a manual backport
#32953 had no
To releaselabel, so no automatic pick was attempted. The cherry-pick has three conflicts, all caused by earliermainchanges that2.0does not have:mainreads events withChangeEventJsonUtils.readOrNull(from fix(alerts): retire the nine EventType values no emitter produces #33033). Here the existingJsonUtils.readValuestays and only the filter call changes.mainrenamedthreadSubject()tofeedSubject()in Fixes #30908: Add Conversation V2 and activity replies #30909. The new helper callsthreadSubject()here.AlertsRuleEvaluatorTestSuiteDomainTest: this test file came with Fixes #30908: Add Conversation V2 and activity replies #30909 and does not exist on2.0, so its update is dropped.Apart from those three adaptations, the added and removed lines match #32953 exactly, plus one review follow-up:
getFilteredEventskeeps its original stream instead of the loop #32953 introduced. The same change goes tomainin #33287, so both branches stay identical. The changedAlertUtilmethods are additions only, and nothing in Collate2.0calls them.Could the resolutions change existing behaviour?
The new check skips a read only when the entity's schema lacks the field. The read itself validates fields against that same list and throws for exactly those fields. So nothing that reads successfully today is skipped.
How it was tested
spotless:checkclean; 100/100 unit tests green across the alert filter, event consumer and subscription scheduler tests, including the two new test classes.2.0.2stack. Three alerts onall: domainFinance, owneradmin, and domainMarketingas a control. Each scenario wrote an event the filter cannot evaluate and a matching table event back to back, so both land in one batch:2.0without itdomainupdate + table update (domain filter)Batch Size : 2error)userupdate + table update (owner filter)domain+ thread about the table (domain filter)user+ thread about the table (owner filter)Marketingcontrol alert/diagnosticInfoduring the batchIn the run without this PR, the thread scenarios failed inside
threadSubjectMatchesDomainandthreadSubjectMatchesOwner, the code changed by the conflict resolution. With it, both delivered threads were the ones about the table.Not in this PR
On
2.0, a domain filter on a test case never falls back to its test suite's domain, because the suites are taken from the event payload, which carries no domains.mainfixed that inside #30909, so the dropped test cannot pass here with or without this change. It is a separate, pre-existing issue.