-
Notifications
You must be signed in to change notification settings - Fork 4
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
Retry backend execute on concurrent append #303
Draft
JCZuurmond
wants to merge
24
commits into
main
Choose a base branch
from
internal/retry-execute-on-concurrent-append
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from 9 commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
22d9c63
Add test with concurrent delta append
JCZuurmond 1f4bb5a
Move roll over method out
JCZuurmond 1745911
Rename table
JCZuurmond 92394ea
Assert the right way
JCZuurmond 1010236
Use test table
JCZuurmond 65dca14
Add comment explaining rollover
JCZuurmond bffd2d5
Retry concurrent append
JCZuurmond 1f0b0a8
Fix string concat
JCZuurmond 44513b3
Fix return type hint of retryable
JCZuurmond 75050ad
Rename variables
JCZuurmond ac3e37c
Remove if delta missing raise as data loss
JCZuurmond 3a7d5b3
Simplify create table
JCZuurmond c83f117
Use `make_table` fixture
JCZuurmond 4844218
Remove wait until roll over
JCZuurmond f5e4db0
Remove unused import
JCZuurmond 3f1c004
Move integration test to the appropriate module
JCZuurmond ae1ee5b
Put back raise error for missing delta transaction log
JCZuurmond 7c0d4f4
Introduce custom `DeltaConcurrentAppend` error
JCZuurmond c31b679
Unit test `DeltaConcurrentAppend` error on statement execution backend
JCZuurmond 664796b
Narrow test
JCZuurmond 4f944ad
Test `DeltaConcurrentAppend` error on `RuntimeBackend`
JCZuurmond 79cb07a
Narrow tests
JCZuurmond b2f2c8c
Format
JCZuurmond 7f3d72e
Add integration test for concurrent write through runtime backend
JCZuurmond File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nfx : Is this what you thinking off?
I have to think about the implications of always retrying this in lsql, maybe we should only retry within UCX isntead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there needs to be a flag to control this, and should default to off: it's not necessarily safe to blindly retry arbitrary SQL.
In general any time we do a 'read-modify-write' cycle, everything needs to start again from the
read
part because themodify
(andwrite
) often depend on it. Sometimes theread
andmodify
bits are within the same SQL statement as thewrite
, in which case this is safe. But often this is part of application code before we get to SQL and that may need to be restarted. In this situation only the application knows what to do.Irrespective of this, whatever we do here also needs to end up in the
.save_table()
implementations: these don't all pass through.execute()
and the same thing can happen there.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RuntimeBackend
, separatelyThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will move the retry to the
RuntimeBackend
. What "predefined common exception" would you suggest to throw? I looked at thesdk.errors
and did not see one that really applied here