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

Add default() to SnapshotConfig #19776

Merged
merged 1 commit into from
Sep 12, 2021

Conversation

brooksprumo
Copy link
Contributor

@brooksprumo brooksprumo commented Sep 10, 2021

Problem

SnapshotConfig is about to get more parameters, and updating all its uses can be cumbersome. Adding default() simplifies future PRs.

Summary of Changes

Add default() to SnapshotConfig and then update all its uses.

@codecov
Copy link

codecov bot commented Sep 11, 2021

Codecov Report

Merging #19776 (06b3839) into master (7aa5f6b) will decrease coverage by 0.0%.
The diff coverage is 83.3%.

@@            Coverage Diff            @@
##           master   #19776     +/-   ##
=========================================
- Coverage    82.5%    82.5%   -0.1%     
=========================================
  Files         472      472             
  Lines      132587   132583      -4     
=========================================
- Hits       109511   109491     -20     
- Misses      23076    23092     +16     

@brooksprumo brooksprumo marked this pull request as ready for review September 11, 2021 10:37
snapshot_utils::DEFAULT_FULL_SNAPSHOT_ARCHIVE_INTERVAL_SLOTS,
incremental_snapshot_archive_interval_slots:
snapshot_utils::DEFAULT_INCREMENTAL_SNAPSHOT_ARCHIVE_INTERVAL_SLOTS,
snapshot_archives_dir: PathBuf::default(),
Copy link
Contributor

Choose a reason for hiding this comment

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

is this == snapshot_archives_dir: PathBuf::from("/"),?

Copy link
Contributor Author

@brooksprumo brooksprumo Sep 12, 2021

Choose a reason for hiding this comment

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

Good catch! No, it's basically an empty string, like String::new(), which ends up being basically Vec::new().

In the rpc_service test that sets snapshot_archive_dir and bank_snapshots_dir to PathBuf::from("/"), the values don't matter at all, just that SnapshotConfig is Some and not None.

Copy link
Contributor

@jeffwashington jeffwashington left a comment

Choose a reason for hiding this comment

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

lgtm

@brooksprumo brooksprumo merged commit 62c8bcf into solana-labs:master Sep 12, 2021
@brooksprumo brooksprumo deleted the snapshot-config-default branch September 12, 2021 18:44
dankelleher pushed a commit to identity-com/solana that referenced this pull request Nov 24, 2021
frits-metalogix added a commit to identity-com/solana that referenced this pull request Nov 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants