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
Import export trie log #6363
Import export trie log #6363
Changes from 36 commits
16c0a49
7dd4928
bf2b098
e67ae51
9b4e0c9
0b9fe83
426848e
7401b59
1b7fb72
11e6b05
f2d01e2
04f1aaa
2f01c5a
56e4c8e
78561b0
42c72cf
9961fc2
9389540
e3d4fbc
c7144fe
20b0ba5
586ab25
67e6f3d
b9640e5
3bc1878
d47ddf5
999edb6
1699fe4
e679cb3
5d3b4f2
f839b75
0caa4cf
2d5d31d
37df23e
5ce1800
98423dc
cf3a5e6
c759bba
5fb9413
087c54b
4b033a3
3a89ac3
9be7d13
75d1c3b
5eb4cda
5425075
2de0b19
248a776
55d653e
c9d38f0
b9d7620
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.
this might make more sense to have first/last block numbers included in the filename. Otherwise it won't be clear what is actually in the files after an export
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 batch filenames aren't used as part of import/export subcommands the filename is taken from command line args instead. This is only used for the prune subcommand
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.
Non blocking feedback -
I am all for code reuse, but if we are going to allow for arbitrary import and export, the import files should be more readable and "createable".
The ObjectOutputStream seems fine for backup/recovery of a pruning process, but when part of a general import/export process this file format is too inscrutable IMO.
At least for import/export we should serialize/deserialize these as json maps. Key as the hash string, and the trielog itself as hex (or as a rich json object if we wanted to be super transparent). In addition to being a bit more introspectable, it would allow us to create and import our own handcrafted trielogs when debugging
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.
nit: not sure how important this is so feel free to ignore it doesn't make sense. But having the format in RLP to match the storage in the database trielog column family might make comparison easier without the extra encoding of the IdentityMap. Depends on the purpose of this tool.
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've added this subcommands to allow extraction and insertion of trie logs mostly for debug or troubleshooting purposes, but a version that imports and exports RLP could be very useful too.
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'd lean towards a more readable format rather than binary as it's hard to work with. Agree RLP is a more useful middle ground than custom binary (if that's what this is).
e.g. I wonder if this could be used to import test data...compare with RlpBlockImporter.java and JsonBlockImporter
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'd rather have us to extend the current feature to allow the export to RLP reusing this feature if necessary, it isn't quite simple to do that right now and it would impact the sub-command prune logic as well.
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.
This is the pruning description not the export description
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.
thanks for catching this
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.
This description looks like the pruning description
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.
thanks for catching this!
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.
Can the exported file contain more than one trielog? Would of expected the import to specify the filename and just import all trielogs in that file
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 at the moment, this could be extended in the future to do so if needed.
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.
rename to blockHash? the variable name suggests this is the hash of the trielog
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.
Could be useful to be able to specify a block number instead of block hash. But only a suggestion not sure which one we will use more in practice
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 block hash is prob better is this case, as you could eventually request by number and not have the right block hash if your node forked at that specific block requested.