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

(LedgerStore/FIFO) Refactor FIFO options and sanity check #23131

Merged
merged 1 commit into from
Feb 19, 2022
Merged

(LedgerStore/FIFO) Refactor FIFO options and sanity check #23131

merged 1 commit into from
Feb 19, 2022

Conversation

yhchiang-sol
Copy link
Contributor

@yhchiang-sol yhchiang-sol commented Feb 15, 2022

Summary of Changes

This PR introduces BlockstoreRocksFifoOptions and performs a sanity check on FIFO options.

When the sanity check fails, blockstore will panic instead of falling back to level compaction.
This will allow users to easily identify and fix any misconfiguration.

Dependency: #23103

@yhchiang-sol yhchiang-sol requested a review from steviez February 15, 2022 03:30
@yhchiang-sol yhchiang-sol changed the title (LedgerStore/FIFO) Panic when the specified cf_size <= FIFO_WRITE_BUFFER_SIZE (LedgerStore/FIFO) Refactor FIFO options and sanity check Feb 15, 2022
Copy link
Contributor

@steviez steviez left a comment

Choose a reason for hiding this comment

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

In general, I like your paradigm shift to error out if they specify bad values instead of falling back on level

ledger/src/blockstore_db.rs Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 17, 2022

Codecov Report

Merging #23131 (df28c0d) into master (c696944) will decrease coverage by 0.0%.
The diff coverage is 68.1%.

@@            Coverage Diff            @@
##           master   #23131     +/-   ##
=========================================
- Coverage    81.3%    81.3%   -0.1%     
=========================================
  Files         570      570             
  Lines      155438   155445      +7     
=========================================
- Hits       126452   126441     -11     
- Misses      28986    29004     +18     

@yhchiang-sol yhchiang-sol requested a review from steviez February 18, 2022 05:44
@yhchiang-sol yhchiang-sol merged commit 8c872e9 into solana-labs:master Feb 19, 2022
Copy link
Contributor

@steviez steviez left a comment

Choose a reason for hiding this comment

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

Just realized I didn't check the "approve" button with my last comment; things look good to me tho!

jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Mar 4, 2022
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