Conversation
Code Review ✅ ApprovedRestores the stream-based approach in 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 |
✅ Playwright Results — workflow succeededValidated commit ✅ 295 passed · ❌ 0 failed · 🟡 0 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) 29m 56s ⏱️ Max setup 5m 28s · max shard execution 13m 43s · max shard-job elapsed before upload 16m 52s · reporting 3s 🌐 214.29 requests/attempt · 1.98 app boots/UI scenario · 37.06% common-shard skew Optimization targets still in progress:
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
🚦 Removed from the merge queue —
|
🚦 Removed from the merge queue —
|
Follow-up to #32953. Fixes #31331
What this changes
#32953 changed
AlertUtil.getFilteredEventsso that it checks each change event on its own: if checking one event throws, only that event is left out. Along the way it replaced the originalentrySet().stream().filter(...).collect(toMap(...))with aHashMapand aforloop. The loop was not needed, and review on the2.0backport (#33282) asked for the stream to come back.This PR restores the stream. The only difference from the code before #32953 is that the filter calls
isChangeEventAllowed, which catches failures, instead ofcheckIfChangeEventIsAllowed, which can throw.Why behaviour does not change
isChangeEventAllowednever throws. It catches the filter's exception, and a separate guard catches anything thrown by the error handler. So the stream can't be cut short, and it keeps the same events the loop kept.Collectors.toMaprejects null values and duplicate keys. The map values are always non-null, and a map's entries can't have duplicate keys. The original code already usedtoMap.How it was tested
spotless:applyclean.AlertUtilFilterIsolationTest,AlertsRuleEvaluatorUndeclaredFieldTest,AbstractEventConsumerTest,EventSubscriptionSchedulerTest,AlertUtilTest. The isolation tests cover a batch where one event fails and a batch where the error handler itself also fails.The same change is applied to the
2.0backport in #33282, so both branches stay identical.