Skip to content
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

Nessie: Replace CommitStateUnknownException with CommitFailedException #4592

Closed

Conversation

nastra
Copy link
Contributor

@nastra nastra commented Apr 20, 2022

This is a follow-up from #4491

@nastra nastra force-pushed the nessie-replace-commitstateunknown-ex branch from 26c7355 to e2dee94 Compare April 20, 2022 06:02
@@ -274,12 +273,9 @@ public void renameTable(TableIdentifier from, TableIdentifier to) {
// to catch all kinds of network errors (e.g. connection reset). Network code implementation
// details and all kinds of network devices can induce unexpected behavior. So better be
// safe than sorry.
throw new CommitStateUnknownException(ex);
throw new CommitFailedException(ex, "Cannot rename table '%s' to '%s': " +
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is right, I think this was originally added because some of the exceptions here do happen when the commit succeeded on the server but it failed to propagate the ack back to the client. At least that's what I was told when this got added way back when. If things have changed let me know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems you have more historical background than I do and in fact you are totally right, because that's exactly the case that's happening here. Closing the PR then

@nastra nastra closed this Apr 20, 2022
@nastra nastra deleted the nessie-replace-commitstateunknown-ex branch April 20, 2022 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants