-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
ledger-tool: Move blockstore commands to blockstore subcommand #34597
Conversation
9f7021c
to
4a41f70
Compare
1cf2c02
to
bae1793
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #34597 +/- ##
=========================================
- Coverage 81.8% 81.8% -0.1%
=========================================
Files 824 824
Lines 222393 222393
=========================================
- Hits 181984 181924 -60
- Misses 40409 40469 +60 |
bae1793
to
f34a07e
Compare
f34a07e
to
a4eba32
Compare
Ok, I think I have pulled out everything that I can to shrink this PR. That being said, still a pretty decent line count. But, I think commits are broken up pretty well such that review shouldn't be too insane. Let me know if this is not the case @CriesofCarrots and we can brainstorm on how to break this up even further. I spot-checked a few items, ie that the following two calls both work:
I had previously spot-checked every command, but will probably do that again tomorrow just incase. It could be nice to have a unit-test, but that would seemingly entail
So, I'm initially leaning towards no unit test, but open to discussion |
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! I diffed almost all the move blocks and didn't come up with anything to comment on :)
let starting_slot_arg = Arg::with_name("starting_slot") | ||
.long("starting-slot") | ||
.value_name("SLOT") | ||
.takes_value(true) | ||
.default_value("0") | ||
.help("Start at this slot"); | ||
let ending_slot_arg = Arg::with_name("ending_slot") | ||
.long("ending-slot") | ||
.value_name("SLOT") | ||
.takes_value(true) | ||
.help("The last slot to iterate to"); | ||
let allow_dead_slots_arg = Arg::with_name("allow_dead_slots") | ||
.long("allow-dead-slots") | ||
.takes_value(false) | ||
.help("Output dead slots 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.
Could we/should we dedupe these with the existing definitions in main.rs at some point?
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.
Yeah definitely; it is not expressed here, but I loosely allude to wanting to do some reorganization on that front in the problem descriptions for #34694 (a PR I have yet to add you too 😉).
The goals there would be deduping argument definitions (there were a few dupes prior to my changes here), and also creating several groupings of arguments. The groupings could be added to subcommands via this function to avoid long repeated lists like this for the [verify, create-snapshot, graph, ...]
commands:
solana/ledger-tool/src/main.rs
Lines 1368 to 1392 in 8438544
.arg(&no_snapshot_arg) | |
.arg(&account_paths_arg) | |
.arg(&accounts_hash_cache_path_arg) | |
.arg(&accounts_index_path_arg) | |
.arg(&halt_at_slot_arg) | |
.arg(&limit_load_slot_count_from_snapshot_arg) | |
.arg(&accounts_index_bins) | |
.arg(&accounts_index_limit) | |
.arg(&disable_disk_index) | |
.arg(&accountsdb_skip_shrink) | |
.arg(&accountsdb_verify_refcounts) | |
.arg(&accounts_db_test_skip_rewrites_but_include_in_bank_hash) | |
.arg(&verify_index_arg) | |
.arg(&accounts_db_skip_initial_hash_calc_arg) | |
.arg(&ancient_append_vecs) | |
.arg(&halt_at_slot_store_hash_raw_data) | |
.arg(&hard_forks_arg) | |
.arg(&accounts_db_test_hash_calculation_arg) | |
.arg(&os_memory_stats_reporting_arg) | |
.arg(&allow_dead_slots_arg) | |
.arg(&max_genesis_archive_unpacked_size_arg) | |
.arg(&debug_key_arg) | |
.arg(&geyser_plugin_args) | |
.arg(&use_snapshot_archives_at_startup) | |
.arg( |
Problem
solana-ledger-tool
is a bit of a kitchen sink, and has upwards of 30 commands at the top level (ie 30 commands that can be invoked viasolana-ledger-tool <SOME_COMMAND>
). UI Aside, the contents ofledger-tool/src/main.rs
are somewhat cluttered.Summary of Changes
To reduce the clutter, this PR:
solana-ledger-tool slot
is nowsolana-ledger-tool blockstore slot
solana-ledger-tool slot
), but does not display those commands in the top level--help
command.With this PR, here is the subcommand section of the output for running
solana-ledger-tool help
:And here is
solana-ledger-tool blockstore
:Pre-requisite PRs
In order to reduce the noise in this PR, several items that I changed while working on this have been broken out. All below PR's should be merged and this PR rebased on top before it is ready to merge.