Skip to content
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: Bubble up error enum instead of eprintln!() and exit() #34426

Merged
merged 1 commit into from
Dec 13, 2023

Conversation

steviez
Copy link
Contributor

@steviez steviez commented Dec 12, 2023

Problem

The ledger-tool load_and_process_ledger() function performs many sub-operations that can fail. The current error handling of printing text and exiting inline adds extra noise to the code that actually does work. Additionally, all of the separate exit points mean that there might be slightly different variations the common text when an unrecoverable error occurs.

Summary of Changes

  • Introduce LoadAndProcessLedgerError enum for load_and_process_ledger() error conditions
  • Remove early exits and instead bubble up a Result<_, LoadAndProcessLedgerError>
  • Introduce and use wrapper that unwraps or exits load_and_process_ledger()
  • Use the wrapper for all previous callers of load_and_process_ledger() to remove boilerplate code

The diff on this one is massive because some match statements were removed and cargo fmt reduced a lot of indentation. All of the changes introduced by cargo fmt are in a single commit, so viewing this PR commit-by-commit OR with whitespace hidden (or both) would be my advice.

All of the struckthrough items will be handled in a separate PR

@steviez steviez marked this pull request as ready for review December 12, 2023 23:24
Copy link

codecov bot commented Dec 12, 2023

Codecov Report

Merging #34426 (efbd735) into master (337c233) will increase coverage by 0.0%.
Report is 2 commits behind head on master.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #34426   +/-   ##
=======================================
  Coverage    81.8%    81.9%           
=======================================
  Files         819      819           
  Lines      221037   221037           
=======================================
+ Hits       181028   181052   +24     
+ Misses      40009    39985   -24     

brooksprumo
brooksprumo previously approved these changes Dec 13, 2023
Copy link
Contributor

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm. Love the error changes. Also somewhat surprised by the number of cargo fmt changes.

Comment on lines +56 to +60
const PROCESS_SLOTS_HELP_STRING: &str =
"The starting slot is either the latest found snapshot slot, or genesis (slot 0) if the \
--no-snapshot flag was specified or if no snapshots were found. \
The ending slot is the snapshot creation slot for create-snapshot, the value for \
--halt-at-slot if specified, or the highest slot in the blockstore.";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤌

ledger-tool/src/ledger_utils.rs Outdated Show resolved Hide resolved
ledger-tool/src/main.rs Outdated Show resolved Hide resolved
@steviez
Copy link
Contributor Author

steviez commented Dec 13, 2023

Also somewhat surprised by the number of cargo fmt changes.

Yeah, definitely an indicator that we need to break some stuff into sub-functions. For example, the implementation of create-snapshot is >350 lines after load_and_process_ledger() and it was all within this match statement that I have removed

solana/ledger-tool/src/main.rs

Lines 3010 to 3018 in 39f2866

match load_and_process_ledger(
arg_matches,
&genesis_config,
blockstore.clone(),
process_options,
snapshot_archive_path,
incremental_snapshot_archive_path,
) {
Ok((bank_forks, starting_snapshot_hashes)) => {

Similarly, capitalization command looks to have 500 lines after it calls load_and_process_ledger(). These two are most of the bulk.

After typing the above comments about line counts out, I think I'm going to split this into two PR's:

  • The first PR will have the first commit that changes eprintln!() & exit!() to bubble up the new error enum
  • The second PR will have the other 3 commits
    • The new helper that reduces repeated handling (ie match statements at call site) / using the new helper / running cargo fmt to fix indentation

I don't think we'd backport any of this functionality, but in any case, I think there is a logical split with these two, and it makes the change that actually does a little more (the error enum one) a little less "obfuscated" by being in the same squashed commit as ~1500 lines of cargo fmt

Copy link
Contributor

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me. Can you update the PR's title before merging, please?

@steviez steviez changed the title ledger-tool: Condense error text and early exits ledger-tool: Bubble up error enum instead of eprintln!() and exit() Dec 13, 2023
@steviez steviez merged commit 48046b6 into solana-labs:master Dec 13, 2023
45 checks passed
@steviez steviez deleted the lt_condense_exits branch December 13, 2023 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants