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

Support entry version in Write batch #1310

Merged
merged 6 commits into from
Apr 21, 2020
Merged

Conversation

jarifibrahim
Copy link
Contributor

@jarifibrahim jarifibrahim commented Apr 17, 2020

This PR adds support for setting different versions for different keys in write batch.
The existing implementation of write batch allows setting only single version for all keys in the write batch.
With this PR, user can do the following

wb := db.NewManagedWriteBatch()
wb.SetEntryAt(e, ts)
wb.Flush()

Also, the existing behavior of txn.Commit() in un-managed (normal) mode is to panic, the new behavior would be to return an error.


This change is Reviewable

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm: Approved. Do simplify as discussed. @gja can do the final approval.

Reviewed 8 of 8 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ashish-goswami)

Copy link
Contributor

@gja gja left a comment

Choose a reason for hiding this comment

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

One more small commit, but otherwise :lgtm:

Reviewable status: 7 of 8 files reviewed, 1 unresolved discussion (waiting on @ashish-goswami, @jarifibrahim, and @manishrjain)


txn.go, line 475 at r2 (raw file):

	}

	keepTogether := true

I see this logic duplicated twice. Move to txn.keepTogether() please

Copy link
Contributor

@gja gja left a comment

Choose a reason for hiding this comment

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

Small Comment sorry

Reviewable status: 7 of 8 files reviewed, 1 unresolved discussion (waiting on @ashish-goswami, @jarifibrahim, and @manishrjain)

Copy link
Contributor Author

@jarifibrahim jarifibrahim left a comment

Choose a reason for hiding this comment

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

Reviewable status: 7 of 8 files reviewed, 2 unresolved discussions (waiting on @ashish-goswami, @gja, and @manishrjain)


txn.go, line 475 at r2 (raw file):

Previously, gja (Tejas Dinkar) wrote…

I see this logic duplicated twice. Move to txn.keepTogether() please

One of the instances of keepTogether sets the version for keys, another one just checks if the version is set.


txn.go, line 478 at r2 (raw file):

	for _, e := range txn.pendingWrites {
		if e.version == 0 {
			e.version = commitTs

Even if there was a single keepTogether function, we would need to keep this loop to assign commitTs to zero-version entries.

Copy link
Contributor

@gja gja left a comment

Choose a reason for hiding this comment

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

Reviewable status: 7 of 8 files reviewed, 2 unresolved discussions (waiting on @ashish-goswami, @jarifibrahim, and @manishrjain)


txn.go, line 475 at r2 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

One of the instances of keepTogether sets the version for keys, another one just checks if the version is set.

Oops, that's my bad. :lgtm:

@jarifibrahim jarifibrahim merged commit fd693e4 into master Apr 21, 2020
@jarifibrahim jarifibrahim deleted the ibrahim/txn-version branch April 21, 2020 06:24
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