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

fix(Transaction): discard empty transactions on CommitWith #2031

Merged
merged 2 commits into from
Dec 18, 2023

Conversation

Wondertan
Copy link
Contributor

@Wondertan Wondertan commented Nov 25, 2023

Problems

  • Transactions with empty pendingWrites were never discarded(and marked as done) for CommitWith

Solution

Make sure Discard is called for CommitWith when pendingWrites are empty. The existing unit test is updated to assert that.

Copy link

netlify bot commented Nov 25, 2023

Deploy Preview for badger-docs canceled.

Name Link
🔨 Latest commit f09ebf1
🔍 Latest deploy log https://app.netlify.com/sites/badger-docs/deploys/6570f26e807a7a00078d535c

@CLAassistant
Copy link

CLAassistant commented Nov 25, 2023

CLA assistant check
All committers have signed the CLA.

@Wondertan
Copy link
Contributor Author

cc @billprovince @joshua-goldstein

@Wondertan
Copy link
Contributor Author

cc @harshil-goel @mangalaman93

txn.go Outdated Show resolved Hide resolved
txn_test.go Outdated Show resolved Hide resolved
@Wondertan Wondertan changed the title fix(Transaction): properly discard transactions during commit fix(Transaction): discard empty transactions on CommitWith Dec 6, 2023
@Wondertan
Copy link
Contributor Author

Ok, so I think I misunderstood the purpose of precommitCheck, and transactions should not be discarded when it errors. I still think the code in Commit funcs is rather confusing and could be simplified to have a single defer, but that's definitely not part of this PR, so I shortened the solution to a one-liner, as well updated unit tests that checks for the bug

@Wondertan
Copy link
Contributor Author

Ping again

@mangalaman93 mangalaman93 merged commit 09b73f7 into dgraph-io:main Dec 18, 2023
11 checks passed
mYmNeo added a commit to mYmNeo/badger that referenced this pull request Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants