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 conflict detection for managed DB #1716

Merged
merged 2 commits into from
Jul 8, 2021

Conversation

roysc
Copy link
Contributor

@roysc roysc commented Jun 25, 2021

I propose this simple fix for detecting conflicts in managed mode. Addresses https://discuss.dgraph.io/t/fatal-error-when-writing-conflicting-keys-in-managed-mode/14784.

When a write conflict exists for a managed DB, an internal assert can fail:

badger/txn.go

Line 616 in aaab253

y.AssertTrue(commitTs != 0)

This occurs because a detected conflict is indicated with commitTs of 0, but handling the error is skipped for managed DB instances:

badger/txn.go

Line 563 in aaab253

if commitTs == 0 && !txn.db.opt.managedTxns {
.

Rather than conflate conflict detection with a timestamp of 0, it can be indicated with another return value from hasConflict.


This change is Reviewable

@CLAassistant
Copy link

CLAassistant commented Jun 25, 2021

CLA assistant check
All committers have signed the CLA.

@roysc roysc marked this pull request as ready for review July 1, 2021 09:53
@roysc roysc requested a review from manishrjain as a code owner July 1, 2021 09:53
Copy link
Contributor

@NamanJain8 NamanJain8 left a comment

Choose a reason for hiding this comment

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

Thanks @roysc, for the fix. This looks good to me.

@roysc roysc closed this Jul 5, 2021
@roysc roysc reopened this Jul 5, 2021
@NamanJain8 NamanJain8 merged commit 3279e18 into dgraph-io:master Jul 8, 2021
NamanJain8 pushed a commit that referenced this pull request Jul 8, 2021
I propose this simple fix for detecting conflicts in managed mode. Addresses https://discuss.dgraph.io/t/fatal-error-when-writing-conflicting-keys-in-managed-mode/14784.

When a write conflict exists for a managed DB, an internal assert can fail.
This occurs because a detected conflict is indicated with commitTs of 0, but handling the error is skipped for managed DB instances.

Rather than conflate conflict detection with a timestamp of 0, it can be indicated with another return value from hasConflict.

(cherry picked from commit 3279e18)
NamanJain8 pushed a commit that referenced this pull request Jul 8, 2021
I propose this simple fix for detecting conflicts in managed mode. Addresses https://discuss.dgraph.io/t/fatal-error-when-writing-conflicting-keys-in-managed-mode/14784.

When a write conflict exists for a managed DB, an internal assert can fail.
This occurs because a detected conflict is indicated with commitTs of 0, but handling the error is skipped for managed DB instances.

Rather than conflate conflict detection with a timestamp of 0, it can be indicated with another return value from hasConflict.

(cherry picked from commit 3279e18)
mergify bot pushed a commit to cosmos/cosmos-sdk that referenced this pull request Sep 15, 2021
## Description

Partially resolves: vulcanize#14

Implements a [BadgerDB](https://pkg.go.dev/github.com/dgraph-io/badger/v3)-based backend for the DB interface introduced by #9573 and specified by [ADR-040](https://github.com/cosmos/cosmos-sdk/blob/eb7d939f86c6cd7b4218492364cdda3f649f06b5/docs/architecture/adr-040-storage-and-smt-state-commitments.md). This uses Badger's "managed" mode for version management, and supports optimistically concurrent transactions (required one [patch](dgraph-io/badger#1716) upstream to fix handling of write conflicts).

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [x] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [x] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - n/a
- [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [x] added a changelog entry to `CHANGELOG.md`
- [x] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [x] updated the relevant documentation or specification
- [x] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
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.

3 participants