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

Delta Lake: Improve error message related to storage layers which allow write collisions #14904

Conversation

findinpath
Copy link
Contributor

@findinpath findinpath commented Nov 4, 2022

Description

Fixes #14464

Non-technical explanation

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

@cla-bot cla-bot bot added the cla-signed label Nov 4, 2022
@findinpath findinpath changed the title Improve error message related to storage layers which allow write collisions Delta Lake: Improve error message related to storage layers which allow write collisions Nov 4, 2022
@alexjo2144
Copy link
Member

This seems like something that should go in the docs, not in an error message. An individual user doesn't have the ability to turn this on, like they do with session properties.

It should also come along with a warning that writes are safe as long as you only have one cluster doing them.

@findinpath
Copy link
Contributor Author

@alexjo2144 what about using this error message:

Inserts are not enabled on the %s filesystem.

?

From a usability point of view, we should give somehow the hint in the message which can be used by sysadmin to adapt the connector configuration if needed.

@mdesmet
Copy link
Contributor

mdesmet commented Nov 8, 2022

It should also come along with a warning that writes are safe as long as you only have one cluster doing them.

According to my understanding when competing writes are happening, the order of execution is random and therefore also the result, even when only one cluster is used. So enabling should probably come with a warning in any case.

I personally think it is user-friendly to also mention the property to set. We do the same in

throw new TrinoException(ICEBERG_FILESYSTEM_ERROR, format("" +
"Cannot create a table on a non-empty location: %s, set 'iceberg.unique-table-location=true' in your Iceberg catalog properties " +
"to use unique table locations for every table.", location));
}

@hashhar
Copy link
Member

hashhar commented Nov 8, 2022

According to my understanding when competing writes are happening, the order of execution is random and therefore also the result, even when only one cluster is used.

That's how transaction failures work on most systems with any isolation level higher than "repeatable read". If multiple queries hold a table lock only one of them will win but which one isn't guaranteed. The failed one has to be retried.

You don't notice it as often with other databases since the default isolation is generally never "repeatable read".

(I do agree we should explain this to users - just wanted to add context to which users of other systems may relate).

I like how Postgres docs explain it https://www.postgresql.org/docs/current/transaction-iso.html (search for retry transactions due to serialization failures).

@alexjo2144
Copy link
Member

what about using this error message:
Inserts are not enabled on the %s filesystem.

This is better, to me.

@findinpath findinpath force-pushed the delta-improve-dml-concurrent-writes-not-enabled-error-message branch from b380ae0 to bdd966a Compare November 14, 2022 20:36
@findinpath findinpath requested a review from findepi November 14, 2022 20:37
@findepi findepi merged commit c28f676 into trinodb:master Nov 18, 2022
@findepi findepi added the no-release-notes This pull request does not require release notes entry label Nov 18, 2022
@github-actions github-actions bot added this to the 404 milestone Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed no-release-notes This pull request does not require release notes entry
Development

Successfully merging this pull request may close these issues.

Improve error message on DML operations when delta.enable-non-concurrent-writes is false
5 participants