unbridled-41 commented on PR #5969: URL: https://github.com/apache/rocketmq-dashboard/pull/5969#issuecomment-6090858110
### Follow-up review of this fix: a pending episode never has an event row Reviewing the first commit of this PR I found the change was inert on the real path: `MybatisPlusAlertStateRepository.toActiveState` returned `Optional.empty()` whenever the state's `rmq_system_alert` row was missing, but a **PENDING** episode never writes one — only `FIRING`/`REMINDER`/`RESOLVED` are emitted (`NativeAlertProcessor` calls `emitLifecycleEvent` on those transitions only). So the very states this PR wants the reconcile loop to end were the ones `findActive` silently dropped, and the bug it reports (a pending anchor surviving a collection gap so the rule fires on the first sample that returns) was still there. Commit `b0ef4280` fixes that in the same branch (no force-push, the first commit `d9e241cb` is untouched): - `toActiveState` tolerates a missing event row for `PENDING` (and only for `PENDING`); a `FIRING`/`ACKED` state still requires its row, because resolving one emits a `RESOLVED` event that needs the labels the row carries. - The synthetic state reuses `scope.instanceId()`, which `MetricCollectionScope` already guarantees non-blank, instead of an empty id (`ActiveAlertState` rejects a blank instance id). - Ending a pending episode emits nothing, so the empty label map is safe by construction. Two tests in `MybatisPlusAlertStateRepositoryTest` drive the real repository (no alert-row stub): | test | asserts | | --- | --- | | `findActiveReturnsAPendingEpisodeThatHasNoEventRowTest` | a persisted `PENDING` row with `alertMapper.selectList` empty must come back from `findActive` with its `AlertStateKey` intact | | `findActiveStillRequiresAnEventRowForAFiringStateTest` | a `FIRING` row with no event row is still excluded | Red/green, both observed locally: ``` # with the pre-fix toActiveState (alert == null -> Optional.empty()) cd server && mvn -o -B -ntp test -Dtest='MybatisPlusAlertStateRepositoryTest#findActiveReturnsAPendingEpisodeThatHasNoEventRowTest' Tests run: 1, Failures: 1, Errors: 0, Skipped: 0 <<< FAILURE! (BUILD FAILURE) # with b0ef4280 cd server && mvn -o -B -ntp test -Dtest='org.apache.rocketmq.studio.ops.alert.**' Tests run: 330, Failures: 0, Errors: 0, Skipped: 0 BUILD SUCCESS ``` `mvn -o -B -ntp checkstyle:check` → BUILD SUCCESS. Branch `fix/alert-pending-anchor`, HEAD `b0ef4280` (on top of `d9e241cb`). -- 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]
