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

all: make badger/v2 compatible with v1 #1293

Merged
merged 1 commit into from
Apr 9, 2020
Merged

all: make badger/v2 compatible with v1 #1293

merged 1 commit into from
Apr 9, 2020

Conversation

mvdan
Copy link
Contributor

@mvdan mvdan commented Apr 8, 2020

There were two instances of init-time work being incompatible with v1.
That is, if one imported both v1 and v2 as part of a Go build, the
resulting binary would end up panicking before main could run. The
examples below are done with the latest versions of v1 and v2, and a
main.go as follows:

package main

import (
	_ "github.com/dgraph-io/badger"
	_ "github.com/dgraph-io/badger/v2"
)

func main() {}

First, the protobuf package used "pb" as its proto package name. This is
a problem, because types are registered globally with their fully
qualified names, like "pb.Foo". Since both badger/pb and badger/v2/pb
tried to globally register types with the same qualified names, we'd get
a panic:

$ go run .
panic: proto: duplicate enum registered: pb.ManifestChange_Operation

goroutine 1 [running]:
github.com/golang/protobuf/proto.RegisterEnum(...)
	.../go/pkg/mod/github.com/golang/protobuf@v1.3.1/proto/properties.go:459
github.com/dgraph-io/badger/v2/pb.init.0()
	.../badger/pb/pb.pb.go:638 +0x459

To fix this, make v2's proto package fully qualified. Since the
namespace is global, just "v2.pb" wouldn't suffice; it's not unique
enough. "dgraph.badger.v2.pb" seems good, since it follows the Go module
path pretty closely.

The second issue was with expvar, which too uses globally registered
names:

$ go run .
2020/04/08 22:59:20 Reuse of exported var name: badger_disk_reads_total
panic: Reuse of exported var name: badger_disk_reads_total

goroutine 1 [running]:
log.Panicln(0xc00010de48, 0x2, 0x2)
	.../src/log/log.go:365 +0xac
expvar.Publish(0x906fcc, 0x17, 0x9946a0, 0xc0000b0318)
	.../src/expvar/expvar.go:278 +0x267
expvar.NewInt(...)
	.../src/expvar/expvar.go:298
github.com/dgraph-io/badger/v2/y.init.1()
	.../badger/y/metrics.go:55 +0x65
exit status 2

This time, replacing the "badger_" var prefix with "badger_v2_" seems
like it's simple enough as a fix.

Fixes #1208.


This change is Reviewable

There were two instances of init-time work being incompatible with v1.
That is, if one imported both v1 and v2 as part of a Go build, the
resulting binary would end up panicking before main could run. The
examples below are done with the latest versions of v1 and v2, and a
main.go as follows:

	package main

	import (
		_ "github.com/dgraph-io/badger"
		_ "github.com/dgraph-io/badger/v2"
	)

	func main() {}

First, the protobuf package used "pb" as its proto package name. This is
a problem, because types are registered globally with their fully
qualified names, like "pb.Foo". Since both badger/pb and badger/v2/pb
tried to globally register types with the same qualified names, we'd get
a panic:

	$ go run .
	panic: proto: duplicate enum registered: pb.ManifestChange_Operation

	goroutine 1 [running]:
	github.com/golang/protobuf/proto.RegisterEnum(...)
		.../go/pkg/mod/github.com/golang/protobuf@v1.3.1/proto/properties.go:459
	github.com/dgraph-io/badger/v2/pb.init.0()
		.../badger/pb/pb.pb.go:638 +0x459

To fix this, make v2's proto package fully qualified. Since the
namespace is global, just "v2.pb" wouldn't suffice; it's not unique
enough. "dgraph.badger.v2.pb" seems good, since it follows the Go module
path pretty closely.

The second issue was with expvar, which too uses globally registered
names:

	$ go run .
	2020/04/08 22:59:20 Reuse of exported var name: badger_disk_reads_total
	panic: Reuse of exported var name: badger_disk_reads_total

	goroutine 1 [running]:
	log.Panicln(0xc00010de48, 0x2, 0x2)
		.../src/log/log.go:365 +0xac
	expvar.Publish(0x906fcc, 0x17, 0x9946a0, 0xc0000b0318)
		.../src/expvar/expvar.go:278 +0x267
	expvar.NewInt(...)
		.../src/expvar/expvar.go:298
	github.com/dgraph-io/badger/v2/y.init.1()
		.../badger/y/metrics.go:55 +0x65
	exit status 2

This time, replacing the "badger_" var prefix with "badger_v2_" seems
like it's simple enough as a fix.

Fixes #1208.
@CLAassistant
Copy link

CLAassistant commented Apr 8, 2020

CLA assistant check
All committers have signed the CLA.

@mvdan
Copy link
Contributor Author

mvdan commented Apr 8, 2020

/cc @dopey @jarifibrahim

@mvdan
Copy link
Contributor Author

mvdan commented Apr 8, 2020

I thought about adding a test, but that would require adding dgraph v1 as a requirement in dgraph/v2, which I think wouldn't be good. I think that the changes are intuitive enough so as to not require exhaustive regression tests.

@jarifibrahim jarifibrahim changed the title all: make dgraph/v2 compatible with v1 all: make badger/v2 compatible with v1 Apr 9, 2020
Copy link
Contributor

@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.

This is awesome. Thank you so much for fixing this @mvdan. I was looking for a fix for the v1/v2 problem for a while.

Reviewable status: 0 of 3 files reviewed, all discussions resolved (waiting on @ashish-goswami, @jarifibrahim, and @manishrjain)

@jarifibrahim
Copy link
Contributor

This looks like a simple change. Merging it without @manishrjain's approval.

@jarifibrahim jarifibrahim merged commit 8097259 into dgraph-io:master Apr 9, 2020
@dopey
Copy link

dopey commented Apr 17, 2020

@mvdan Thanks a ton for this! Just testing it out now and loading v1 and v2 works like a charm. 🎆

@mvdan
Copy link
Contributor Author

mvdan commented Apr 17, 2020

Happy to have helped :) maybe this is relevant enough to mention in the upcoming changelog.

@jarifibrahim
Copy link
Contributor

Hey @mvdan, I noticed that a new github.com/dgraph-io/badger/v2/pb folder is generated inside pb directory when I run pb/gen.sh script. Is this expected? This wasn't the case before this PR.

$git checkout master
Already on 'master'
Your branch is up to date with 'upstream/master'.

$ls
gen.sh  pb.pb.go  pb.proto

$ ./gen.sh

$git status
On branch master
Your branch is up to date with 'upstream/master'.

Untracked files:
  (use "git add <file>..." to include in what will be committed)

	github.com/

nothing added to commit but untracked files present (use "git add" to track)

Do you know why this directory is being generated? Also, I'm trying to rename the package but I'm getting some compiler error on PR my PR #1314

[11:13:17]build github.com/dgraph-io/badger/v2/badger: cannot load github.com/dgraph-io/badger/v2/pb: module github.com/dgraph-io/badger@latest found (v1.6.1), but does not contain package github.com/dgraph-io/badger/v2/pb
[11:13:17]Process exited with code 1

Do you have any ideas about what I am doing wrong?

@mvdan
Copy link
Contributor Author

mvdan commented Apr 20, 2020

The generator outputs a directory structure following GOPATH, which doesn't really work well when one is using modules. You could just make the script move the file back to its right place and remove the now-empty directories, unless the generator has a way to output the protobuf file in a custom location (the current directory).

@jarifibrahim
Copy link
Contributor

Thanks @mvdan, I was able to fix the issue by moving the file to the correct directory. Thank you! :)

@mholt
Copy link

mholt commented Jun 29, 2020

Would you be willing to do a point release for this?

@dopey
Copy link

dopey commented Jun 29, 2020

A new point release (with this feature) would help me as well. Currently building off a commit.

@jarifibrahim
Copy link
Contributor

Hey @mholt and @dopey, we've identified some issues with the code in master and the release is blocked until the following issues are resolved.

#1387
#1388
#1389

Apologies for not releasing badger sooner but we can't release badger with known crashes :)

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.

Guidance on Badger migration from v1 to v2
5 participants