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 WriteBatch API in managed mode #948

Merged
merged 5 commits into from Aug 3, 2019
Merged

Conversation

jarifibrahim
Copy link
Contributor

@jarifibrahim jarifibrahim commented Jul 29, 2019

This PR adds a new WriteBatchAt(commitTs) API which allows user to use write batch in managed mode.

Fixes #944


This change is Reviewable

@jarifibrahim jarifibrahim marked this pull request as ready for review July 30, 2019 10:47
Copy link
Contributor

@ashish-goswami ashish-goswami 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: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @ashish-goswami, @jarifibrahim, and @manishrjain)


batch.go, line 51 at r1 (raw file):

// NewWriteBatchAt is similar to NewWriteBatch but it allows user to set the commit timestamp.
// NewWriteBatchAt is supposed to be used in the managed mode.
func (db *DB) NewWriteBatchAt(commitTs uint64) (*WriteBatch, error) {

should we move function to managed_db.go?

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: Don't return error. And avoid making other changes.

Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jarifibrahim)


batch.go, line 44 at r1 (raw file):

	if db.opt.managedTxns {
		return nil,
			errors.New("cannot use NewWriteBatch in managed mode. Use NewWriteBatchAt instead")

Make it panic. No need for error. So, don't change any existing APIs.


batch.go, line 54 at r1 (raw file):

	if !db.opt.managedTxns {
		return nil, errors.New(
			"cannot use NewWriteBatchAt with managedDB=false. Use NewTransaction instead")

Make this panic as well.

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: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @ashish-goswami and @manishrjain)


batch.go, line 44 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Make it panic. No need for error. So, don't change any existing APIs.

Done.


batch.go, line 51 at r1 (raw file):

Previously, ashish-goswami (Ashish Goswami) wrote…

should we move function to managed_db.go?

Moved to managed_db.go


batch.go, line 54 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Make this panic as well.

Done.

@jarifibrahim jarifibrahim merged commit 7f43769 into master Aug 3, 2019
@jarifibrahim jarifibrahim deleted the ibrahim/writebatchat branch August 3, 2019 06:47
jarifibrahim pushed a commit that referenced this pull request Aug 9, 2019
This commit adds a new `WriteBatchAt(commitTs)` API which allows the user to use 
write batch in managed mode.

Fixes #944
jarifibrahim pushed a commit that referenced this pull request Mar 12, 2020
This commit adds a new `WriteBatchAt(commitTs)` API which allows the user to use 
write batch in managed mode.

Fixes #944


(cherry picked from commit 7f43769)
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.

badger.WriteBatch doesn't work well with managed table.
3 participants