[PIE-1810] Update export subcommand to export blocks in rlp format #1852
Conversation
| protected void exportBlock(final FileOutputStream outputStream, final Block block) | ||
| throws IOException { | ||
| final BytesValue rlp = RLP.encode(block::writeTo); | ||
| outputStream.write(rlp.getArrayUnsafe()); |
There was a problem hiding this comment.
Question: Are you writing binary data in the file ? Is it what we want ?
There was a problem hiding this comment.
Yep, this matches the format the blocks import accepts
| // throws Exception { | ||
| // | ||
| // final long startBlock = 0L; | ||
| // final long endBlock = 1L; |
There was a problem hiding this comment.
Why are those tests commented ?
| final long endBlock = maybeEndBlock.orElse(blockchain.getChainHeadBlockNumber() + 1L); | ||
| checkArgument(startBlock >= 0 && endBlock >= 0, "Start and end blocks must be greater than 0."); | ||
| checkArgument(startBlock < endBlock, "Start block must be less than end block"); | ||
|
|
There was a problem hiding this comment.
[optional] maybe a check arg that startblock <= chainHead?
| @Option( | ||
| names = "--to", | ||
| required = true, |
There was a problem hiding this comment.
So we don't currently support export to standard output correct? An absent --to option is how I wold expect that to be configured. It may be interesting for json export but it sounds wrong for RLP export.
There was a problem hiding this comment.
Right, no one wants to read binary data from std out :D But yeah, we might need to rethink it for json export. Although given you can potentially export massive amounts of data, doesn't seem like the worst thing to require a file to export to.
| exportRlpFormat(controller); | ||
| break; | ||
| default: | ||
| throw new ParameterException( |
There was a problem hiding this comment.
Nice, this future proofs us against random formats being added to the enum and by effect being exposed to the CLI.
| + ") must be greater start block (" | ||
| + startBlock | ||
| + ")."); | ||
| } |
There was a problem hiding this comment.
We don't know the state of the blockchain here correct? So we can't assert startblock is in the chain? I think this could be a user error we can detect (somehow) and we should provide the user feedback on. Perhaps we could count the number of blocks exported and return it as part of the exportBlocks method, and log a warning or output the console if no blocks were exported.
There was a problem hiding this comment.
We'll currently log a warning if any blocks in the defined range were missing. Might be a good idea to error out if no blocks are exported though ... I may follow-up on that in another PR.
| File databaseDirectory = new File(databasePath.toString()); | ||
| if (!databaseDirectory.exists() | ||
| || !databaseDirectory.isDirectory() | ||
| || databaseDirectory.list().length == 0) { |
There was a problem hiding this comment.
isDirectory implies exists so exists can be removed.
There was a problem hiding this comment.
I don't think this is more readable but just for fun, the whole thing can be determined from list() because it returns null if it's not a directory or doesn't exist :P
Optional.ofNullable(databaseDirectory.list()).filter(files -> files.length == 0).isPresent()
PR description
Update blocks export subcommand to export blocks in rlp format. Rearrange import / export utilities so that these classes are grouped together with consistent naming.