-
Notifications
You must be signed in to change notification settings - Fork 113
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
Add helper to go from BoltDB -> Bbolt #23
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having the transition function would be handy indeed to ease and standardize migrations across all of our products. Thanks for taking it on!
v2/bolt_store.go
Outdated
//Start a connection to the old | ||
oldtx, err := oldconn.Begin(false) | ||
if err != nil { | ||
return newbolt, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The convention is to return a nil
object if there is an error:
return newbolt, err | |
return nil, err |
Though, error handling requires more care here: we should also close the newbolt
and delete it.
Maybe a stretch goal: It'd be great if if the target file isn't created until the data transfer succeeds. This is typically done by using a temp file in the target directory, then renaming it to the final path the end of the function. @rboyer has a library for atomic Renames: https://pkg.go.dev/github.com/rboyer/safeio , that I believe consul uses for this pattern.
v2/bolt_store.go
Outdated
|
||
boltdbbolt "github.com/boltdb/bolt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be v1
or something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very good. Few minor comments remaining; it might make sense to have another set of 👀 to review before merging.
v2/bolt_store_test.go
Outdated
if err := sourceDb.StoreLogs(logs); err != nil { | ||
t.Fatalf("failed storing logs in source database: %s", err) | ||
} | ||
sourceResult := new(raft.Log) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the destDB
shortening. Maybe use src
prefix for source
as well?
It's still kind of messy - so I'll be updating with some cleanup.
There are two buckets which logs are stored in, dbConf and dbLogs.
We create those buckets Bbolt and then iterate through BoltDBs logs, putting them into Bbolt's.
v1 Conn had to be made public so we can connect to it from v2