-
Notifications
You must be signed in to change notification settings - Fork 15
feat(cloning): make cloning concurrency configurable #473
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
base: dev
Are you sure you want to change the base?
Conversation
1e94ab6
to
98e249d
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'm not sure if this belongs into AccountsCloneConfig
since it is related to ledger replay.
We already have a LedgerConfig
and I'd add a field named either of the following there:
account_hydration_concurrency|clone_concurrency|hydration_clone_concorrency` (or if you have a better name).
We could either add that directly or (preferred) in a LedgerReplayConfig
, so the toml would look similar to.
[ledger]
replay = { account_hydration_concurrency = 20 }
Also please add a fixture here and a test that shows we can parse these new configs from toml properly.
Also please always include a usage example, i.e. a toml snippet, ENV var setting + arg override. That makes it much easier to see what config was added and how.
Addressed in 5a1313b |
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.
LGTM!
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.
In general LGTM I'm confused by the args/env vars:
cargo run --release -- --replay-hydration-concurrency 10
REPLAY_HYDRATION_CONFIG=10 cargo run --release
shouldn't that be:
cargo run --release -- --ledger-replay-hydration-concurrency 10
LEDGER_REPLAY_HYDRATION_CONFIG=10 cargo run --release
Otherwise how do we disambiguate between fields that exist on different sub configs?
Also even if lengthy I'd prefer account_hydration_concurrency
even though either is fine especially if we provide a sample toml with comments which explains what is being hydrated.
Closes #275
It introduces a parameter to choose the degree of concurrency for cloning accounts on hydration. The default stays the same, but this can now be adjusted
Snippets
In TOML configs:
With the CLI: