Skip to content

[omdb] remove full scans in webhook delivery list, refactoring#10758

Merged
hawkw merged 2 commits into
mainfrom
eliza/fix-omdb-webhooks-full-scan
Jul 8, 2026
Merged

[omdb] remove full scans in webhook delivery list, refactoring#10758
hawkw merged 2 commits into
mainfrom
eliza/fix-omdb-webhooks-full-scan

Conversation

@hawkw

@hawkw hawkw commented Jul 7, 2026

Copy link
Copy Markdown
Member

Depends on #10753.

It turns out the query in omdb db alert webhook delivery list is also very full-table-scan-y, so this branch basically gives it the same treatment as I gave alert list in #10753. I also made similar clap changes here, so we get nice help text for possible values.

@hawkw
hawkw requested review from mergeconflict and smklein July 7, 2026 00:49
Comment thread dev-tools/omdb/src/bin/omdb/db/alert.rs Outdated
Comment thread nexus/db-queries/src/db/datastore/webhook_delivery.rs
Comment on lines +686 to +688
rx_lookup.lookup_for(authz::Action::Read).await.with_context(
|| format!("looking up alert receiver {receiver} failed"),
)?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this handles --include-deleted. As far as I can tell, the old lookup_webhook_rx didn't handle it either, and webhook_rx_delete hard-deletes the receiver's children anyway, but maybe there's some inconsistency here that wasn't intended.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, because these are hard-deleted, there isn't really a nice way to implement include_deleted for this command. i'm not too worried about that.

Comment thread nexus/db-queries/src/db/datastore/webhook_delivery.rs
Base automatically changed from eliza/fix-omdb-alerts-full-scan to main July 7, 2026 21:01
Depends on #10753.

It turns out the query in `omdb db alert webhook delivery list` is also
very full-table-scan-y, so this branch basically gives it the same
treatment as I gave `alert list` in #10753. I also made similar clap
changes here, so we get nice help text for possible values.
@hawkw
hawkw force-pushed the eliza/fix-omdb-webhooks-full-scan branch from ab41491 to 99b2af1 Compare July 7, 2026 21:06
@hawkw

hawkw commented Jul 7, 2026

Copy link
Copy Markdown
Member Author

seems check_opte_ver has yet again gotten a spurious 403 from the GitHub API: https://github.com/oxidecomputer/omicron/actions/runs/28900778918/job/85736732771?pr=10758#step:5:25

@hawkw
hawkw enabled auto-merge (squash) July 7, 2026 21:52
@hawkw
hawkw merged commit f006c42 into main Jul 8, 2026
20 of 21 checks passed
@hawkw
hawkw deleted the eliza/fix-omdb-webhooks-full-scan branch July 8, 2026 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants