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

channeldb: isolate migrations #3633

Merged
merged 6 commits into from Nov 2, 2019

Conversation

joostjager
Copy link
Collaborator

@joostjager joostjager commented Oct 24, 2019

Fixes #3628

joostjager added 2 commits Oct 24, 2019
This commit is a direct copy of the complete channeldb package. It only
changes the package declaration at the top of every file. We make this
full copy so that review can be focused on the actual changes made.
Otherwise changes may drown in all the file moves.

Linting for the new package is disabled, as it contains lots of
pre-existing issues.
This commit removes the migrations from channeldb and references those
in the migrations_01_to_11 package. This creates a one-way dependency on
the migrations. Future changes to channeldb won't be able to break
migrations anymore.
@joostjager joostjager changed the title Safe migrations channeldb: isolate migrations Oct 24, 2019
@joostjager joostjager added this to WIP in v0.9.0-beta Oct 24, 2019
@joostjager joostjager self-assigned this Oct 24, 2019
@joostjager joostjager requested review from halseth and Roasbeef Oct 24, 2019
@joostjager joostjager moved this from WIP to Needs Review in v0.9.0-beta Oct 24, 2019
@joostjager joostjager added this to the 0.9.0 milestone Oct 24, 2019
Copy link
Member

@Roasbeef Roasbeef left a comment

Seems like this diff can be much smaller (only copy the essentials).


// TestChannelCache checks the behavior of the channelCache with respect to
// insertion, eviction, and removal of cache entries.
func TestChannelCache(t *testing.T) {
Copy link
Member

@Roasbeef Roasbeef Oct 29, 2019

Choose a reason for hiding this comment

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

Do we really need to copy over all the tests as well? Can't we just copy the call graph of any encode/decode functions used in the migration.

Copy link
Collaborator Author

@joostjager joostjager Oct 29, 2019

Choose a reason for hiding this comment

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

Previously I found that I needed some test code because it was reused. Left all of it in, but I think that also had a lot of deps that prevents more removal of code. Will look at the diff again.

Copy link
Collaborator Author

@joostjager joostjager Oct 29, 2019

Choose a reason for hiding this comment

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

Ok, removed many more lines. Down to 5k lines now.

I used staticcheck from https://github.com/dominikh/go-tools with the -unused.whole-program flag to find more unused code.

@joostjager joostjager force-pushed the safe-migrations branch 4 times, most recently from 8114062 to 838ffb0 Compare Oct 29, 2019
@joostjager joostjager requested a review from Roasbeef Oct 29, 2019
@joostjager
Copy link
Collaborator Author

@joostjager joostjager commented Oct 29, 2019

Unit tests failing. Even by only removing code, I seem to have broken something. Will investigate.

@joostjager
Copy link
Collaborator Author

@joostjager joostjager commented Oct 30, 2019

Test fixed. Problem was only an uninitialized logger. Not something magical triggered by code removal. Ready for review again.

Copy link
Collaborator

@halseth halseth left a comment

not pretty, but I think it works. LGTM 🐸

channeldb/migration_01_to_11/README.md Outdated Show resolved Hide resolved
channeldb/migration_01_to_11/db.go Outdated Show resolved Hide resolved
@@ -48,12 +48,6 @@ type UnknownElementType struct {
element interface{}
}

// NewUnknownElementType creates a new UnknownElementType error from the passed
// method name and element.
func NewUnknownElementType(method string, el interface{}) UnknownElementType {
Copy link
Collaborator

@halseth halseth Oct 30, 2019

Choose a reason for hiding this comment

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

ok, guess the code is removed here.

Copy link
Collaborator Author

@joostjager joostjager Oct 30, 2019

Choose a reason for hiding this comment

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

Not sure what this refers to. Probably you reviewed a previous version of the pr?

channeldb/db.go Show resolved Hide resolved
@joostjager joostjager force-pushed the safe-migrations branch 2 times, most recently from 2f8e88f to a29bdce Compare Oct 30, 2019
@joostjager joostjager requested a review from cfromknecht Oct 30, 2019

// FetchMeta fetches the meta data from boltdb and returns filled meta
// structure.
func (d *DB) FetchMeta(tx *bbolt.Tx) (*Meta, error) {
Copy link
Collaborator

@halseth halseth Oct 31, 2019

Choose a reason for hiding this comment

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

Is this code used by anything now?

Also I wouldn't mind removing tests, since they could depend on code that otherwise could be deleted.

Copy link
Collaborator Author

@joostjager joostjager Oct 31, 2019

Choose a reason for hiding this comment

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

The code is used to do syncVersion for migration tests. But that isn't really necessary to test every time. Added a commit that removes it.

Non-migration tests are all removed (afaik), the _test.go files that remain just contain some vars and helpers that are used in the migration tests.

@halseth halseth self-requested a review Oct 31, 2019
Copy link
Collaborator

@cfromknecht cfromknecht left a comment

Latest version LGTM

halseth
halseth approved these changes Nov 1, 2019
@joostjager joostjager moved this from Needs Review to Approved in v0.9.0-beta Nov 1, 2019
Copy link
Member

@Roasbeef Roasbeef left a comment

LGTM 💎

@Roasbeef Roasbeef merged commit 38e313a into lightningnetwork:master Nov 2, 2019
1 of 2 checks passed
v0.9.0-beta automation moved this from Approved to Done Nov 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v0.9.0-beta
  
Done
Development

Successfully merging this pull request may close these issues.

4 participants