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.

Sign up for GitHub

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

Jump to bottom

refactor: split backends into packages, add metadb #149

Closed
wants to merge 1 commit into from
Closed

refactor: split backends into packages, add metadb #149

wants to merge 1 commit into from

Conversation

mvdan
Copy link
Contributor

@mvdan mvdan commented Feb 7, 2021 •
edited
Loading

(see commit message, which is updated with pushes)

@mvdan mvdan requested review from melekes and tessr as code owners February 7, 2021 21:02
@mvdan
Copy link
Contributor Author

mvdan commented Feb 7, 2021

Alright, here's v1 of the huge refactor. One test is still being picky, I'll figure that out tomorrow. Otherwise this should all work - thoughts welcome.

It should go without saying that this is a big breaking change, but updating the users shouldn't be too hard. It should really be just replacing db.NewDB(...) calls with metadb.NewDB(...) to keep the old behavior with build tags.

melekes
melekes approved these changes Feb 8, 2021
View reviewed changes
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

❤️ great work @mvdan

internal/dbtest/dbtest.go Show resolved Hide resolved
internal/dbtest/dbtest.go
return buf
}

func Bytes2Int64(buf []byte) int64 {
Copy link
Contributor

Choose a reason for hiding this comment

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

godoc is missing

@melekes
Copy link
Contributor

melekes commented Feb 8, 2021

Looks like some tests are failing:

# github.com/tendermint/tm-db/metadb [github.com/tendermint/tm-db/metadb.test]
metadb/db_cleveldb.go:14: undefined: metadb in metadb.RegisterDBCreator
metadb/db_cleveldb.go:14: undefined: clevelDBbCreator
FAIL	github.com/tendermint/tm-db/metadb [build failed]
# github.com/tendermint/tm-db/metadb
FAIL	github.com/tendermint/tm-db/remotedb [build failed]
metadb/db_cleveldb.go:14:15: undefined: metadb
metadb/db_cleveldb.go:14:57: undefined: clevelDBbCreator
FAIL	github.com/tendermint/tm-db/remotedb/grpcdb [build failed]

@mvdan
Copy link
Contributor Author

mvdan commented Feb 8, 2021

Yeah, I realise some godocs are missing and the tests aren't passing - I was hoping to get an early review before I spend more time polishing it. The overall refactor sounds good, then?

I don't think it's worth adding godocs to the internal test funcs, though. They didn't have it before, and they are still internal test APIs.

@mvdan mvdan closed this Feb 8, 2021
@mvdan mvdan reopened this Feb 8, 2021
@tac0turtle
Copy link
Contributor

Does it make sense to remove the name of the db from the file name? It's already in the respective folder. Badgerdb/badger_db.go could be badgerdb/db.go.

@melekes
Copy link
Contributor

melekes commented Feb 8, 2021

Does it make sense to remove the name of the db from the file name? It's already in the respective folder. Badgerdb/badger_db.go could be badgerdb/db.go.

Or it can be just badger/db.go.

@melekes
Copy link
Contributor

melekes commented Feb 8, 2021

The overall refactor sounds good, then?

yep

@mvdan
Copy link
Contributor Author

mvdan commented Feb 8, 2021

Or it can be just badger/db.go.

I actually named the packages foodb instead of foo for a good reason; to keep the package names explicit. That is also why I rename tm-db to tmdb in the code instead of using just db, which is far too generic. Similarly, package names like mem, meta, remote, or golevel would be pretty confusing.

I agree with reducing repetition with filenames, though. I thought about doing the same with types, renaming badgerdb.BadgerDB to just badgerdb.DB, but I didn't want to refactor yet more things in this already-huge PR.

@mvdan
refactor: split backends into packages, add metadb
786fcf1 
The old structure was a single huge package with all the DB backends in
it. Since tm-db users often want just one or two DB backends, and
building plus linking all of them is very heavy, build tags were used to
opt into some of the backends.

This same mechanism is now in metadb, for those who want it; it allows
opting into DB backends via build tags, without having to edit the code
directly.

Note that the old package included the memdb and goleveldb backends by
default. The new metadb does not include any by default, to make
backends explicit and more consistent. This required using more build
tags in CI.

Most users will want to statically use one DB implementation, without
metadb. Each separate sub-package does just that. They are direct moves
of the old code; the only major change is that constructors like
db.NewBadgerDB are now called badger.NewDB, to avoid the repetition of
badger.NewBadgerDB.

The root package error types are also exported now, to allow being used
by all separate packages. Note that this means they're now part of the
public API too; this should be fine.

Some test code also had to be shuffled around. All the tests which dealt
with backends were moved to metadb. The shared test helper code had to
be moved to an internal package, to remain reachable.

Finally, note that we consistently use "tmdb" as the imported package
name for the root package, since "db" is often used as a variable name.
This prevents confusion and common build errors.

Fixes #113.
@mvdan
Copy link
Contributor Author

mvdan commented Feb 9, 2021

Alright, all tests should pass now. I've also updated CI and the makefile with the new memdb and goleveldb build tags.

@mvdan
Copy link
Contributor Author

mvdan commented Feb 10, 2021

I'm not sure what to do about the CI failures. The test failures are signal: illegal instruction (core dumped), which I cannot reproduce. golangci-lint seems to fail at loading a package due to missing Cgo, which I also don't really understand as I don't use golangci-lint.

@tac0turtle
Copy link
Contributor

tac0turtle commented Feb 10, 2021 •
edited
Loading

It seems to error within the docker image. I was able to reproduce the issue with this command

test-all-docker:
	@echo "--> Running go test"
	@docker run --rm -v $(CURDIR):/workspace --workdir /workspace tendermintdev/docker-tm-db-testing go test $(PACKAGES) -tags memdb,goleveldb,cleveldb,boltdb,rocksdb,badgerdb -v
.PHONY: test-all-docker

@mvdan
Copy link
Contributor Author

mvdan commented Feb 10, 2021

I see. I can't fix that as I don't own the image. Maybe I should revert the CI changes and let one of you do that after this PR is merged. Or feel free to push follow-up commits on this same PR if you want to fix it here.

@tac0turtle
Copy link
Contributor

The docker file is located in /tools, if you'd like to take a stab, otherwise reverting is fine. We can take a look later. The image will automatically be deployed since the file was touched.

I would have to do some testing, I'm unsure why its failing.

@tac0turtle tac0turtle mentioned this pull request Feb 11, 2021
Merged
@tac0turtle
Copy link
Contributor

It's odd that I need to download rocksdb to run the linter.

@tac0turtle
Copy link
Contributor

this PR fixes it: #151

not sure how linting fixed the tests.

@tac0turtle
Copy link
Contributor

tac0turtle commented Feb 11, 2021 •
edited
Loading

closing this in favor of #151

thank you, @mvdan, for the contribution I closed this in favor of the PR with the Ci fixes

@tac0turtle tac0turtle closed this Feb 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@melekes melekes melekes approved these changes

@tessr tessr Awaiting requested review from tessr

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mvdan @melekes @tac0turtle