-
Notifications
You must be signed in to change notification settings - Fork 43
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
feat: Add simple data import and export #1630
feat: Add simple data import and export #1630
Conversation
6a710f3
to
cb93a0a
Compare
Codecov ReportPatch coverage:
@@ Coverage Diff @@
## develop #1630 +/- ##
===========================================
- Coverage 75.46% 75.27% -0.19%
===========================================
Files 203 208 +5
Lines 21092 21694 +602
===========================================
+ Hits 15916 16330 +414
- Misses 4082 4221 +139
- Partials 1094 1143 +49
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
cb93a0a
to
0e1ee1b
Compare
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 think the documentation at least needs to change, documenting the memory cost of this operation, if the team is happy with the current implementation in the short-term.
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.
Is a nice first step and I think provides quality of life improvements for users.
I do have a slight nitpick for the terminology of backup/recover
over import/export
but I think that might be just me :P
suggestion: Would be nice to perhaps include a simple cli usage example in the README.md
0e1ee1b
to
5577321
Compare
Note: The tests will fail as I didn't update them for the most recent changes. I want to ensure that we agree with the approach before I put time into the tests which I'll do as soon as consensus is reached. The change moves the handling of the import and export to the Let me know what you guys think :) |
5577321
to
97f4a2c
Compare
e6a0c1a
to
cc38c53
Compare
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 good, but I'd like a few more changes before merge please. I'm only part way through review, but thought I'd submit what I have now and let you get cracking.
9d06362
to
2307340
Compare
513d731
to
61ef8ce
Compare
dcc6b04
to
27e1e6b
Compare
@@ -387,6 +389,25 @@ defradb start --allowed-origins=http://localhost:3000 | |||
|
|||
The catch-all `*` is also a valid origin. | |||
|
|||
## Backing up and restoring |
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.
thought: I like this backing up and restoring analogy better :), but wondering if should be consistent as currently we have files with the following names:
backup_export.go
backup_import.go
Maybe instead can be:
backup.go
restore.go
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 way I went about it is: it's the backup feature and it can export and import data. I wanted a category of actions.
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.
Yep I get that, I feel like backup is being used as an umbrella term. IMO commands will be simpler too if we can pick up one concise word for each action. As pointed out below
## Backing up and restoring | ||
|
||
It is currently not possible to do a full backup of DefraDB that includes the history of changes through the Merkle DAG. However, DefraDB currently supports a simple backup of the current data state in JSON format that can be used to seed a database or help with transitioning from one DefraDB version to another. | ||
|
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.
thought: If the above thought is expanded then we can have the cli be:
defradb client backup path/to/backup.json
defradb client restore path/to/backup.json
Instead of:
defradb client backup export path/to/backup.json
defradb client backup import path/to/backup.json
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 had
defradb client export path/to/backup.json
defradb client import path/to/backup.json
before and it was suggested that grouping them under a command / api path would be clearer.
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.
Hmm, I do like the one with less words to type. But if there seems to be a consensus reached already then will just follow that (also is a non-blocker thought).
## Relevant issue(s) Resolves sourcenetwork#1544 ## Description This PR adds import and export functionality to the http api and cli. It can export to json to reduce the potential file size. At this stage csv output was not implemented as it would require extensive type casting (everything in csv is a string) on both writing and reading from the csv.
Relevant issue(s)
Resolves #1544
Description
This PR adds import and export functionality to the http api and cli. It can export to json to reduce the potential file size. At this stage csv output was not implemented as it would require extensive type casting (everything in csv is a string) on both writing and reading from the csv.
Tasks
How has this been tested?
make test and manual testing
Specify the platform(s) on which this was tested: