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

RethinkDB: Disable transactions that were not working #2733

Merged
merged 2 commits into from Apr 3, 2023

Conversation

ItalyPaleAle
Copy link
Contributor

The implementation of the "Multi" method in RethinkDB was not correct, as it caused operations to be executed in the wrong order and it was not using a transaction (per comment on code: best effort, no transacts supported)

In essence, RethinkDB was not supporting transactions, and advertising the existence of the Multi method can cause users to believe it does, with dangerous potentials for data loss.

The implementation of the "Multi" method in RethinkDB was not correct, as it caused operations to be executed in the wrong order and it was not using a transaction (per comment on code: `best effort, no transacts supported`)

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
@ItalyPaleAle ItalyPaleAle requested review from a team as code owners March 31, 2023 20:08
@ItalyPaleAle ItalyPaleAle added this to the v1.11 milestone Apr 1, 2023
Copy link
Contributor

@mukundansundar mukundansundar left a comment

Choose a reason for hiding this comment

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

potential breaking change, even though it was not working.
also docs needs to be changed.

@mukundansundar mukundansundar added breaking-change documentation required This issue needs documentation labels Apr 3, 2023
@ItalyPaleAle
Copy link
Contributor Author

also docs needs to be changed.

Docs did not mention RethinkDB as supporting transactions: https://docs.dapr.io/reference/components-reference/supported-state-stores/

Because of that, and the fact that it's beta, I think this is not a breaking change (or at least not one we should be concerned with).

@ItalyPaleAle ItalyPaleAle removed breaking-change documentation required This issue needs documentation labels Apr 3, 2023
@ItalyPaleAle ItalyPaleAle merged commit c65319b into dapr:master Apr 3, 2023
82 of 84 checks passed
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.

None yet

2 participants