Skip to content

fix: improve delete_metrics_splits error handling and doc comments#6280

Merged
g-talbot merged 4 commits intomainfrom
gtt/fix-delete-metrics-splits
Apr 8, 2026
Merged

fix: improve delete_metrics_splits error handling and doc comments#6280
g-talbot merged 4 commits intomainfrom
gtt/fix-delete-metrics-splits

Conversation

@g-talbot
Copy link
Copy Markdown
Contributor

@g-talbot g-talbot commented Apr 8, 2026

Summary

  • Rewrite delete_metrics_splits in postgres metastore to distinguish "not found" (warn + succeed) from "not deletable" (FailedPrecondition), matching the non-metrics delete_splits pattern
  • Fix row_keys_proto doc comment to use plain code span instead of broken rustdoc link

Split out from #6247 (docs PR) where these code changes didn't belong.

Test plan

  • Existing metastore tests pass
  • Postgres delete tests cover not-found and not-deletable cases

🤖 Generated with Claude Code

@mattmkim
Copy link
Copy Markdown
Contributor

mattmkim commented Apr 8, 2026

@codex review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fcf8e650a7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +2642 to +2645
LEFT JOIN metrics_splits
ON metrics_splits.index_uid = $1
AND metrics_splits.split_id = input_splits.split_id
),
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Lock metrics rows before deriving not_deletable state

delete_metrics_splits computes input_splits from metrics_splits without FOR UPDATE, unlike the non-metrics delete_splits flow it is mirroring. Under concurrent mark_metrics_splits_for_deletion updates, this statement can evaluate an older split_state snapshot and return FailedPrecondition for splits that were already transitioned to MarkedForDeletion by a racing transaction, causing spurious delete failures and retry churn. Adding row locking in the state-read CTE (as done in delete_splits) avoids this stale-state race.

Useful? React with 👍 / 👎.

@g-talbot g-talbot merged commit 71f0d30 into main Apr 8, 2026
8 checks passed
@g-talbot g-talbot deleted the gtt/fix-delete-metrics-splits branch April 8, 2026 21:09
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