fix: retry advancement of PreparedCommit into PostCommit in case version 0 already exists (created by another writer)
#3513
Conversation
danielgafni
requested review from
roeap,
rtyler,
hntd187 and
ion-elgreco
as code owners
|
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
changed the title
PreparedCommit into PostCommit in case version 0 has been created (by another writer)PreparedCommit into PostCommit in case version 0 has been created (by another writer)
|
What is the DCO check complaining about? My commit has been signed (even confirmed by GitHub). Does it expect a specific format?
|
|
@danielgafni do git commit -m "msg" -s |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3513 +/- ##
==========================================
- Coverage 74.14% 74.09% -0.05%
==========================================
Files 150 150
Lines 44550 44552 +2
Branches 44550 44552 +2
==========================================
- Hits 33031 33013 -18
- Misses 9382 9384 +2
- Partials 2137 2155 +18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
| Err(TransactionError::VersionAlreadyExists(0)) => Ok(()), // this can happen if the table has been created by another writer since the `this.table_data.is_none()` check above | ||
| Err(e) => Err(e), | ||
| }?; | ||
| } | ||
|
|
||
| // unwrap() is safe here due to the above check |
There was a problem hiding this comment.
You need to also load the snapshot now when VersionAlreadyExists was thrown the first time in it's current state this will panic.
There was a problem hiding this comment.
Uhh right. Done.
Can we test this? Seems tricky since we'd have to perfectly time writing the 0 version from another writer.
There was a problem hiding this comment.
Should we also initialize attempt_number to 2 in this case?
There was a problem hiding this comment.
Would be easier to test in Rust
There was a problem hiding this comment.
Can you add a test for this please? There are some concurrency tests, somewhere in the rust codebase which you can use as template
There was a problem hiding this comment.
I'll look into that. A more direct pointer would be appreciated
There was a problem hiding this comment.
delta-rs/crates/test/src/concurrent.rs here you can find some related stuff
|
@ion-elgreco I do have commit signing enabled by default, and GitHub recognizes them as well (see the green Edit: actually this is the case --- the |
danielgafni
force-pushed
the
retry-inial-table-creation
branch
from
b4dc03a to
cf61e65
Compare
Signed-off-by: danielgafni <[email protected]>
danielgafni
force-pushed
the
retry-inial-table-creation
branch
from
cf61e65 to
98605fc
Compare
danielgafni
changed the title
PreparedCommit into PostCommit in case version 0 has been created (by another writer)PreparedCommit into PostCommit in case version 0 already exists (created by another writer)
Description
This PR fixes the
PreparedCommitto fallback to the retry mechanism in case version 0 has been created by another writer.Related Issue(s)
Resolve #3505