Skip to content

fix: Use CatalogCommitConflicts for TableRequirement #1532

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jul 22, 2025

Conversation

CTTY
Copy link
Contributor

@CTTY CTTY commented Jul 20, 2025

Which issue does this PR close?

What changes are included in this PR?

  • Use ErrorKind::CatalogCommitConflicts for TableRequirement::check
  • When metadata is None, returns ErrorKind::TableNotFound
  • Minor trimmings

Are these changes tested?

No

Copy link
Contributor

@liurenjie1024 liurenjie1024 left a comment

Choose a reason for hiding this comment

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

Thanks @CTTY for this pr, generally LGTM! Left one comment to address.

@@ -232,7 +234,7 @@ impl Error {
// internally. It's zero cost if backtrace is disabled.
backtrace: Backtrace::capture(),

retryable: false,
retryable: kind == ErrorKind::CatalogCommitConflicts,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is odd to me, I would expect explicit call to change the retryable property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I don't have a strong opinion on this, was just thinking it will be more convenient like this. Will fix this

Copy link
Contributor

@liurenjie1024 liurenjie1024 left a comment

Choose a reason for hiding this comment

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

Thanks @CTTY for this fix! LGTM!

@liurenjie1024 liurenjie1024 merged commit a675031 into apache:main Jul 22, 2025
26 of 27 checks passed
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.

TableRequirement check failure should return CatalogCommitConflicts
2 participants