Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 tool for database conversion #2061
Add tool for database conversion #2061
Changes from 46 commits
b6ae081
291214b
a52d9ac
67922b5
5f61bc3
689e653
83a22e1
a1ea3ae
0868fa2
d14a201
dd4ec96
c81e6c8
97ea9b7
1e7041e
c735874
83f1f57
31e5b8c
e779fe7
d7f5360
bc41c66
62c86a5
39a5311
dd33ae2
c82675a
48f5c84
7cb6417
59bdbfc
c3b4a19
c3c6aff
7e6e2d1
0062b76
9daf165
d7dfdb8
073c40e
b213597
fc98de6
d7f5ea9
5eac1d4
88e8221
e308cf6
8657e51
09d0371
c47ee34
bc8b85d
25ab55d
897a33e
7f9f7ef
6595a44
cfb1393
6a22f1d
a2d3507
dc24202
bc3d784
37a826e
0ca882a
a5ebac9
1d69881
7527848
77eb4da
8422e35
21f2ee4
7895656
8ec8389
ee7cba8
6a9e8ff
4ebfd7a
b0484c5
53c448f
4d79dfc
5b61070
3fdab93
a78fb97
44f0e18
bc8803a
93aaaef
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
nitpick:
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.
included in: #2591
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.
Not sure why is useful to add those field to this struct.
Those values are built and closed in the same function call, e.g.,
Convert
opens a ``dst` in the beginning of its execution, and it is closed in the end of its execution.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.
good catch, removed those
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.
Why we don't convert the freezer?
Would be interesting to add a comment here explaining that?
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 freezer has format independent of db-engine, it only needs to be copied or moved
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.
Return err if is non nil here, before updating stats.
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.
nitpick: converting
config.Verify
from an int to an enum here can improve code readability in places whereconfig.Verify
is being usedThere 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've changed the config to string and as it's not used in many places I didn't convert it to an enum, just used it as string
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.
We should check if err is non nil here
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.
nitpick: unnecessary line
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.
addressed in: #2591
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.
nitpick: unnecessary, since conv.Convert already has a defer c.Close