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

Vendor badger with the latest changes. #3606

Merged
merged 3 commits into from
Jun 28, 2019
Merged

Conversation

martinmr
Copy link
Contributor

@martinmr martinmr commented Jun 26, 2019

Also update Dgraph to use the new badger Options API.


This change is Reviewable

Need the changes to backup.go to keep working on the backup data format
changes.
@martinmr martinmr requested review from manishrjain and a team as code owners June 26, 2019 17:08
@martinmr martinmr removed the request for review from manishrjain June 26, 2019 17:09
campoy
campoy previously requested changes Jun 26, 2019
Copy link
Contributor

@campoy campoy left a comment

Choose a reason for hiding this comment

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

Reviewed 38 of 38 files at r1.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @danielmai and @martinmr)


conn/node_test.go, line 38 at r1 (raw file):

func openBadger(dir string) (*badger.DB, error) {
	opt := badger.DefaultOptions(dir)

I would drop this function altogether and replace it with

badger.Open(badger.DefaultOptions(dir))

Or maybe just make this whole function body one line


dgraph/cmd/debug/run.go, line 746 at r1 (raw file):

		isWal = true
	}
	bopts := badger.DefaultOptions(dir).WithTableLoadingMode(options.MemoryMap).WithReadOnly(

I would write this a bit different and do one WithX per line, but it's totally stylistic so I won't push


dgraph/cmd/live/run.go, line 251 at r1 (raw file):

		var err error
		db, err = badger.Open(o)

Replace these lines with

	db, err := badger.Open(badger.DefaultOptions(opt.clientDir).
		WithTableLoadingMode(bopt.MemoryMap).
		WithSyncWrites(false))

ee/backup/run.go, line 224 at r1 (raw file):

		bo := badger.DefaultOptions(dir).WithSyncWrites(true).WithTableLoadingMode(
			options.MemoryMap).WithValueThreshold(1 << 10).WithNumVersionsToKeep(math.MaxInt32)
		db, err := badger.OpenManaged(bo)

Again, no need to declare bo as a variable, instead you can write

	db, err := badger.OpenManaged(badger.DefaultOptions(dir).
		WithSyncWrites(true).
		WithTableLoadingMode(options.MemoryMap).
		WithValueThreshold(1 << 10).
		WithNumVersionsToKeep(math.MaxInt32))

posting/list_test.go, line 1251 at r1 (raw file):

	opt := badger.DefaultOptions(dir)
	ps, err = badger.OpenManaged(opt)
ps, err := badger.OpenManaged(badger.DefaultOptions(dir))

raftwal/storage_test.go, line 50 at r1 (raw file):

func openBadger(dir string) (*badger.DB, error) {
	opt := badger.DefaultOptions(dir)
	return badger.Open(opt)

Same as in previous comment, this function is not really necessary


vendor/github.com/dgraph-io/badger/appveyor.yml, line 16 at r1 (raw file):

  GOVERSION: 1.8.3
  GOPATH: c:\gopath
  GO111MODULE: on

Why is this?

Copy link
Contributor Author

@martinmr martinmr 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: all files reviewed, 7 unresolved discussions (waiting on @campoy and @danielmai)


conn/node_test.go, line 38 at r1 (raw file):

Previously, campoy (Francesc Campoy) wrote…

I would drop this function altogether and replace it with

badger.Open(badger.DefaultOptions(dir))

Or maybe just make this whole function body one line

Done.


dgraph/cmd/debug/run.go, line 746 at r1 (raw file):

Previously, campoy (Francesc Campoy) wrote…

I would write this a bit different and do one WithX per line, but it's totally stylistic so I won't push

Done.


dgraph/cmd/live/run.go, line 251 at r1 (raw file):

Previously, campoy (Francesc Campoy) wrote…

Replace these lines with

	db, err := badger.Open(badger.DefaultOptions(opt.clientDir).
		WithTableLoadingMode(bopt.MemoryMap).
		WithSyncWrites(false))

Done.


ee/backup/run.go, line 224 at r1 (raw file):

Previously, campoy (Francesc Campoy) wrote…

Again, no need to declare bo as a variable, instead you can write

	db, err := badger.OpenManaged(badger.DefaultOptions(dir).
		WithSyncWrites(true).
		WithTableLoadingMode(options.MemoryMap).
		WithValueThreshold(1 << 10).
		WithNumVersionsToKeep(math.MaxInt32))

Done.


posting/list_test.go, line 1251 at r1 (raw file):

Previously, campoy (Francesc Campoy) wrote…
ps, err := badger.OpenManaged(badger.DefaultOptions(dir))

Done.


raftwal/storage_test.go, line 50 at r1 (raw file):

Previously, campoy (Francesc Campoy) wrote…

Same as in previous comment, this function is not really necessary

Done.


vendor/github.com/dgraph-io/badger/appveyor.yml, line 16 at r1 (raw file):

Previously, campoy (Francesc Campoy) wrote…

Why is this?

I have no idea. I thought this was a change you made.

@martinmr martinmr requested a review from campoy June 26, 2019 23:35
@martinmr martinmr dismissed campoy’s stale review June 26, 2019 23:35

addressed review

Copy link
Contributor

@campoy campoy left a comment

Choose a reason for hiding this comment

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

Reviewed 9 of 9 files at r2.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @campoy and @danielmai)

@martinmr martinmr merged commit 5e79717 into master Jun 28, 2019
@martinmr martinmr deleted the martinmr/vendor-badger branch June 28, 2019 17:41
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 19, 2019
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.

2 participants