-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[extension/db_storage] Add transaction in batch operations #30551
[extension/db_storage] Add transaction in batch operations #30551
Conversation
When any batch operation fails, the db state should not be changed. To resolve this, the proposal is to add a transaction. Signed-off-by: andrefrco <[email protected]>
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
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.
Changes look good. You might need a changelog entry. Is there a need to support toggling off transactions?
Signed-off-by: andrefrco <[email protected]>
Thanks @atoulme . I added the changelog entry. I can't see in which use case we need to disable the transaction, we expect the entire batch to be processed or nothing. @dmitryax can help us with a second opinion? |
Signed-off-by: andrefrco <[email protected]>
I'll check the lint errors as soon as possible |
Signed-off-by: andrefrco <[email protected]>
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
I agree. I don't think non-transactional behavior is desired at all |
Co-authored-by: Dmitrii Anoshin <[email protected]>
Co-authored-by: Dmitrii Anoshin <[email protected]>
Signed-off-by: andrefrco <[email protected]>
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Description: When any batch operation fails, the db state should not be changed. To resolve this, the proposal is to add a transaction.
Link to tracking Issue: #29730
Testing: I found no reason why there are no
client.go
tests. So I decided to also create it for existing functions. If it doesn't make sense I can remove it.