Skip to content

fix: retry advancement of PreparedCommit into PostCommit in case version 0 already exists (created by another writer) #3513

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: retry advancement of PreparedCommit into PostCommit in case version 0 already exists (created by another writer) #3513

wants to merge 1 commit into from
+96 −17

Conversation

@danielgafni
Copy link

@danielgafni danielgafni commented Jun 2, 2025 •
edited
Loading

Description

This PR fixes the PreparedCommit to fallback to the retry mechanism in case version 0 has been created by another writer.

Related Issue(s)

Resolve #3505

@github-actions github-actions bot added the binding/rust Issues for the Rust crate label Jun 2, 2025
@github-actions
Copy link

github-actions bot commented Jun 2, 2025

ACTION NEEDED

delta-rs follows the Conventional Commits specification for release automation.

The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification.

@danielgafni danielgafni changed the title 🐛 retry advancement of PreparedCommit into PostCommit in case version 0 has been created (by another writer) fix: retry advancement of PreparedCommit into PostCommit in case version 0 has been created (by another writer) Jun 2, 2025
@danielgafni
Copy link
Author

danielgafni commented Jun 2, 2025

What is the DCO check complaining about? My commit has been signed (even confirmed by GitHub). Does it expect a specific format?

image

@ion-elgreco
Copy link
Collaborator

ion-elgreco commented Jun 2, 2025

@danielgafni do git commit -m "msg" -s

@codecov
Copy link

codecov bot commented Jun 2, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 26.98413% with 46 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.08%. Comparing base (948396f) to head (706e131).

Files with missing lines Patch % Lines
crates/test/src/concurrent.rs 0.00% 36 Missing ⚠️
crates/core/src/kernel/transaction/mod.rs 62.96% 10 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##             main    #3513       +/-   ##
===========================================
+ Coverage   26.24%   74.08%   +47.84%     
===========================================
  Files         124      152       +28     
  Lines       19824    39601    +19777     
  Branches    19824    39601    +19777     
===========================================
+ Hits         5203    29340    +24137     
+ Misses      14251     8933     -5318     
- Partials      370     1328      +958

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

ion-elgreco
ion-elgreco reviewed Jun 2, 2025
View reviewed changes
crates/core/src/kernel/transaction/mod.rs Outdated
}?;
}

// unwrap() is safe here due to the above check
Copy link
Collaborator

@ion-elgreco ion-elgreco Jun 2, 2025

Choose a reason for hiding this comment

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

You need to also load the snapshot now when VersionAlreadyExists was thrown the first time in it's current state this will panic.

Copy link
Author

@danielgafni danielgafni Jun 2, 2025

Choose a reason for hiding this comment

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

Uhh right. Done.
Can we test this? Seems tricky since we'd have to perfectly time writing the 0 version from another writer.

Copy link
Author

@danielgafni danielgafni Jun 2, 2025

Choose a reason for hiding this comment

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

Should we also initialize attempt_number to 2 in this case?

Copy link
Collaborator

@ion-elgreco ion-elgreco Jun 2, 2025

Choose a reason for hiding this comment

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

Would be easier to test in Rust

Copy link
Collaborator

@ion-elgreco ion-elgreco Jun 3, 2025

Choose a reason for hiding this comment

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

Can you add a test for this please? There are some concurrency tests, somewhere in the rust codebase which you can use as template

Copy link
Author

@danielgafni danielgafni Jun 3, 2025

Choose a reason for hiding this comment

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

I'll look into that. A more direct pointer would be appreciated

Copy link
Collaborator

@ion-elgreco ion-elgreco Jun 4, 2025

Choose a reason for hiding this comment

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

delta-rs/crates/test/src/concurrent.rs here you can find some related stuff

@danielgafni
Copy link
Author

danielgafni commented Jun 2, 2025 •
edited
Loading

@ion-elgreco I do have commit signing enabled by default, and GitHub recognizes them as well (see the green Verified label next to my commit). This makes me think there is something specific about this DCO check configuration that my signing doesn't do.

Edit: actually this is the case --- the -s option introduces the commit authorship text which was not present previously. Looks like it has to be configured in the commit message template for automatic signing.

@danielgafni danielgafni force-pushed the retry-inial-table-creation branch 2 times, most recently from cf61e65 to 98605fc Compare June 2, 2025 09:51
@danielgafni danielgafni changed the title fix: retry advancement of PreparedCommit into PostCommit in case version 0 has been created (by another writer) fix: retry advancement of PreparedCommit into PostCommit in case version 0 already exists (created by another writer) Jun 2, 2025
@danielgafni danielgafni requested a review from ion-elgreco June 2, 2025 15:32
@rtyler rtyler self-assigned this Jun 18, 2025
@ion-elgreco ion-elgreco force-pushed the retry-inial-table-creation branch from 98605fc to 4ed550b Compare July 5, 2025 10:25
@roeap roeap added this to delta-rust Aug 8, 2025
@roeap roeap moved this to Ready in delta-rust Aug 20, 2025
@danielgafni danielgafni force-pushed the retry-inial-table-creation branch 4 times, most recently from c80dad3 to 7cad22f Compare November 25, 2025 16:48
@danielgafni
Copy link
Author

danielgafni commented Nov 28, 2025

@ion-elgreco could you please take another look?

@danielgafni danielgafni force-pushed the retry-inial-table-creation branch from 7cad22f to c0d9c59 Compare November 28, 2025 21:16
@danielgafni danielgafni force-pushed the retry-inial-table-creation branch from c0d9c59 to 706e131 Compare November 28, 2025 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@roeap roeap Awaiting requested review from roeap roeap is a code owner

@rtyler rtyler Awaiting requested review from rtyler rtyler is a code owner

@hntd187 hntd187 Awaiting requested review from hntd187 hntd187 is a code owner

@ion-elgreco ion-elgreco Awaiting requested review from ion-elgreco

At least 1 approving review is required to merge this pull request.

Assignees

@rtyler rtyler

Labels

binding/rust Issues for the Rust crate

Projects

delta-rust
Status: Ready

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Initial table creation may fail with multiple writers

3 participants

@danielgafni @ion-elgreco @rtyler