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

Make transaction log commit with filesystem storage backend to be atomic #126

Merged
merged 17 commits into from
Mar 18, 2021

Conversation

mosyp
Copy link
Contributor

@mosyp mosyp commented Mar 15, 2021

Description

This PR makes transaction log commit by writing file in put_obj first in temporary file and once that successful then uses atomic rename (with checking on destination existence) to move temporary file into final commit file.

Related Issue(s)

Fixes #94

Documentation

@mosyp mosyp changed the title Add platform/file-system specific rename with flags call Make transaction log commit with filesystem storage backend to be atomic Mar 15, 2021
Copy link
Contributor Author

@mosyp mosyp left a comment

Choose a reason for hiding this comment

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

I've got several concern here

  • If rename fails due to concurrent commits, it'll start over again with incremented version which means that we'll have it writes tmp file again and again. However the impact will be on higher level within try_commit_loop which is out of backend storage lvl.
  • Current impl does not remove tmp files once it fails, but this should be added/revised once previous concern is resolved

rust/src/storage/file.rs Outdated Show resolved Hide resolved
rust/src/storage/file.rs Outdated Show resolved Hide resolved
rust/src/storage/file.rs Outdated Show resolved Hide resolved
rust/src/storage/file.rs Outdated Show resolved Hide resolved
@mosyp mosyp marked this pull request as ready for review March 15, 2021 16:00
rust/src/storage/file.rs Outdated Show resolved Hide resolved
rust/src/storage/file.rs Outdated Show resolved Hide resolved
rust/src/storage/file.rs Outdated Show resolved Hide resolved
rust/src/storage/file.rs Outdated Show resolved Hide resolved
rust/src/storage/file.rs Outdated Show resolved Hide resolved
rust/src/storage/rename.rs Outdated Show resolved Hide resolved
rust/src/storage/file.rs Outdated Show resolved Hide resolved
rust/src/storage/file.rs Outdated Show resolved Hide resolved
@houqp
Copy link
Member

houqp commented Mar 16, 2021

The rest looks good to me 👍 just need to fix that build issue :D

rust/src/storage/file.rs Outdated Show resolved Hide resolved
rust/src/storage/file/mod.rs Outdated Show resolved Hide resolved
rust/src/storage/file/mod.rs Outdated Show resolved Hide resolved
rust/src/storage/file/mod.rs Outdated Show resolved Hide resolved
@xianwill
Copy link
Collaborator

I've got several concern here

  • If rename fails due to concurrent commits, it'll start over again with incremented version which means that we'll have it writes tmp file again and again. However the impact will be on higher level within try_commit_loop which is out of backend storage lvl.
  • Current impl does not remove tmp files once it fails, but this should be added/revised once previous concern is resolved

@mosyp - Regarding 1 you mentioned above, I'll create a separate issue for that problem. Ultimately, we need to expose a rename_obj on the StorageBackend so DeltaTransaction can handle it the same way across storage backends. I.e.

* write temp file
* optimistic commit loop:
  * atomic rename
  * if version already exists
    * do conflict resolution if update, update temp file
  * increase version

@mosyp
Copy link
Contributor Author

mosyp commented Mar 18, 2021

@xianwill yes, that's a good idea to move it out of this scope. I feel like once we're done with s3 atomic rename, then we're safe to create some abstraction which will work for both fs and s3

@xianwill
Copy link
Collaborator

Posted #135

Copy link
Member

@houqp houqp left a comment

Choose a reason for hiding this comment

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

great work @mosyp !

@houqp
Copy link
Member

houqp commented Mar 18, 2021

@xianwill want to take a final look before we merge?

@xianwill
Copy link
Collaborator

LGTM! Great work @mosyp!

@houqp houqp merged commit e7a5fe3 into delta-io:main Mar 18, 2021
@mosyp mosyp deleted the fs-rename branch March 18, 2021 16:48
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.

Transaction log commit with filesystem storage backend is not atomic
3 participants