Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

Gracefully handles errors when archiving snapshots #34856

Merged
merged 1 commit into from
Jan 19, 2024
Merged

Gracefully handles errors when archiving snapshots #34856

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions core/src/snapshot_packager_service.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,14 +71,18 @@ impl SnapshotPackagerService {
// Archiving the snapshot package is not allowed to fail.
// AccountsBackgroundService calls `clean_accounts()` with a value for
// last_full_snapshot_slot that requires this archive call to succeed.
snapshot_utils::archive_snapshot_package(
let result = snapshot_utils::archive_snapshot_package(
&snapshot_package,
&snapshot_config.full_snapshot_archives_dir,
&snapshot_config.incremental_snapshot_archives_dir,
snapshot_config.maximum_full_snapshot_archives_to_retain,
snapshot_config.maximum_incremental_snapshot_archives_to_retain,
)
.expect("failed to archive snapshot package");
);
if let Err(err) = result {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Could do the if statement directly on the function call:

                        if let Err(err) = snapshot_utils::archive_snapshot_package(
                            &snapshot_package,
                            &snapshot_config.full_snapshot_archives_dir,
                            &snapshot_config.incremental_snapshot_archives_dir,
                            snapshot_config.maximum_full_snapshot_archives_to_retain,
                            snapshot_config.maximum_incremental_snapshot_archives_to_retain,
                        ) {
                            error!("Stopping SnapshotPackagerService! Fatal error while archiving snapshot package: {err}");
                            exit.store(true, Ordering::Relaxed);
                            break;                           
                        }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! I actually prefer not this. My brain likes to see the call to archive_snapshot_package() separate; because I find it a bit harder to read when the part of the if before the braces ({}) is longer.

Clippy even has a lint for this: https://rust-lang.github.io/rust-clippy/master/index.html#/blocks_in_conditions, and I fixed a few of them, like here: #34630.

I do agree that this is all subjective, and others prefer the terse/inline-with-the-conditional version instead.

error!("Stopping SnapshotPackagerService! Fatal error while archiving snapshot package: {err}");
exit.store(true, Ordering::Relaxed);
break;
}

if let Some(snapshot_gossip_manager) = snapshot_gossip_manager.as_mut() {
snapshot_gossip_manager.push_snapshot_hash(
Expand Down