feat: add a forest-tool subcommand that generates a merged actor bundle #3325
Conversation
hanabi1224
requested review from
ruseinov and
lemmih
and removed request for
a team
src/mod.rs
Outdated
|
|
||
| pub type BlockPair = (Cid, Vec<u8>); | ||
|
|
||
| pub fn read_car_as_stream<R>(reader: CarReader<R>) -> impl Stream<Item = BlockPair> |
There was a problem hiding this comment.
Let's use the new and improved crate::utils::db::car_stream::CarStream
There was a problem hiding this comment.
Hmm that will require bringing CarStream to the context of build script, might be too much effort
There was a problem hiding this comment.
Oh, I see. Let's change car_cmd.rs to not use the build module.
There was a problem hiding this comment.
Oh, I see. Let's change
car_cmd.rsto not use the build module.
If I understand this correctly - the proposal is to use the new CarStream in car_cmd.rs, while maintaining the old logic in the build script?
When I look at this it seems to me that removing the workspace completely might have been premature. If we'd be able to have this shared logic in another package - we would easily be able to reuse it within the build script. Perhaps a good enough reason to re-introduce the workspace with at least two packages forest-core and forest-util, for example. That would definitely allow us to get rid of mod.rs as per Aatif's suggestion.
There was a problem hiding this comment.
Another option is to assemble the bundle when there's a new network version rather than in the build script.
There was a problem hiding this comment.
Another option is to assemble the bundle when there's a new network version rather than in the build script.
True, there's only one downside to it - anything that's not automated is prone to human error.
I'm fine with either approach as long as we don't have to maintain the outdated build script logic.
hanabi1224
changed the title
| } | ||
|
|
||
| let all_roots = car_streams | ||
| .iter() |
There was a problem hiding this comment.
Not sure if there's anything to be gained in parallelising this. Could be interesting depending on the times it takes to run this.
There was a problem hiding this comment.
roots are part of CAR header which has been parsed in CarStream::new so this call is super fast
|
|
||
| let mut zstd_encoder = ZstdEncoder::with_quality( | ||
| async_fs::File::create(Path::new(DEFAULT_BUNDLE_FILE_NAME)).await?, | ||
| async_compression::Level::Precise(22), |
There was a problem hiding this comment.
Where is this number coming from? I'd at least make it a const and comment on the reasoning.
There was a problem hiding this comment.
22 is the max supported level. I'm not able to find a corresponding const from either zstd or zstd-safe, will make it a const and add a link to the doc in the comment
There was a problem hiding this comment.
Actually there is a function zstd_safe::max_c_level() available, switched
src/cli/subcommands/car_cmd.rs
Outdated
| @@ -55,7 +55,7 @@ impl CarCommands { | |||
| } | |||
| } | |||
|
|
|||
| fn merge_car_streams<R>( | |||
| pub fn merge_car_streams<R>( | |||
There was a problem hiding this comment.
Not a big of importing methods defined in cmd into another crate. Perhaps move them into some shared location? That also goes for dedup_block_stream.
There was a problem hiding this comment.
Moved to car_util.rs
| // Copyright 2019-2023 ChainSafe Systems | ||
| // SPDX-License-Identifier: Apache-2.0, MIT | ||
|
|
||
| //! This module contains code that is shared between the library and the build script |
There was a problem hiding this comment.
I'm curious about this as it does not seem like build.rs has any references to that.
There was a problem hiding this comment.
It used to do, but not anymore, and that's why I remove it in this PR
Summary of changes
Changes introduced in this pull request:
Move shared CAR concat functions intobuildmod to avoid code duplicationforest-tool state-migration actor-bundlesubcommand that generates a merged actor bundlesrc/mod.rsReference issue to close (if applicable)
Closes #3324
Other information and links
Change checklist