From 7fef7d569ae69996080dec025461ce2638b635e6 Mon Sep 17 00:00:00 2001 From: steviez Date: Tue, 4 Oct 2022 12:38:40 -0500 Subject: [PATCH] Make RocksFifo storage size API expose an Option (#28192) A fifo rocksdb instance must be opened with max size parameter on the fifo columns. To support this, we previously plumbed a constant up to callers that provided a default if unbounded growth desired. This change attempts to be more rusty by exposing an option for this value, and converting the option to a constant at the lowest level possible. --- ledger-tool/src/main.rs | 7 +++---- ledger-tool/tests/basic.rs | 8 +++----- ledger/src/blockstore_options.rs | 33 +++++++++++++++++++++----------- validator/src/main.rs | 16 +++++++--------- 4 files changed, 35 insertions(+), 29 deletions(-) diff --git a/ledger-tool/src/main.rs b/ledger-tool/src/main.rs index 5ef44a715a3056..fd310e242c317e 100644 --- a/ledger-tool/src/main.rs +++ b/ledger-tool/src/main.rs @@ -33,7 +33,6 @@ use { blockstore_options::{ AccessType, BlockstoreOptions, BlockstoreRecoveryMode, LedgerColumnOptions, ShredStorageType, BLOCKSTORE_DIRECTORY_ROCKS_FIFO, - MAX_ROCKS_FIFO_SHRED_STORAGE_SIZE_BYTES, }, blockstore_processor::{self, BlockstoreProcessorError, ProcessOptions}, shred::Shred, @@ -875,9 +874,9 @@ fn open_blockstore_with_temporary_primary_access( fn get_shred_storage_type(ledger_path: &Path, warn_message: &str) -> ShredStorageType { // TODO: the following shred_storage_type inference must be updated once // the rocksdb options can be constructed via load_options_file() as the - // temporary use of MAX_ROCKS_FIFO_SHRED_STORAGE_SIZE_BYTES could - // affect the persisted rocksdb options file. - match ShredStorageType::from_ledger_path(ledger_path, MAX_ROCKS_FIFO_SHRED_STORAGE_SIZE_BYTES) { + // value picked by passing None for `max_shred_storage_size` could affect + // the persisted rocksdb options file. + match ShredStorageType::from_ledger_path(ledger_path, None) { Some(s) => s, None => { warn!("{}", warn_message); diff --git a/ledger-tool/tests/basic.rs b/ledger-tool/tests/basic.rs index 3d57d05f583617..8023d39b2edac4 100644 --- a/ledger-tool/tests/basic.rs +++ b/ledger-tool/tests/basic.rs @@ -87,7 +87,6 @@ fn insert_test_shreds(ledger_path: &Path, ending_slot: u64) { } fn ledger_tool_copy_test(src_shred_compaction: &str, dst_shred_compaction: &str) { - const TEST_ROCKS_FIFO_SHRED_STORAGE_SIZE_BYTES: u64 = std::u64::MAX; let genesis_config = create_genesis_config(100).genesis_config; let (ledger_path, _blockhash) = match src_shred_compaction { @@ -101,10 +100,9 @@ fn ledger_tool_copy_test(src_shred_compaction: &str, dst_shred_compaction: &str) let target_ledger_path = get_tmp_ledger_path_auto_delete!(); if dst_shred_compaction == "fifo" { - let rocksdb_fifo_path = target_ledger_path.path().join( - ShredStorageType::rocks_fifo(TEST_ROCKS_FIFO_SHRED_STORAGE_SIZE_BYTES) - .blockstore_directory(), - ); + let rocksdb_fifo_path = target_ledger_path + .path() + .join(ShredStorageType::rocks_fifo(None).blockstore_directory()); fs::create_dir_all(rocksdb_fifo_path).unwrap(); } let target_ledger_path = target_ledger_path.path().to_str().unwrap(); diff --git a/ledger/src/blockstore_options.rs b/ledger/src/blockstore_options.rs index 2c1aaf717fb962..c129046ff18a9d 100644 --- a/ledger/src/blockstore_options.rs +++ b/ledger/src/blockstore_options.rs @@ -142,11 +142,10 @@ pub const BLOCKSTORE_DIRECTORY_ROCKS_LEVEL: &str = "rocksdb"; pub const BLOCKSTORE_DIRECTORY_ROCKS_FIFO: &str = "rocksdb_fifo"; impl ShredStorageType { - /// Returns ShredStorageType::RocksFifo where the specified - /// `shred_storage_size` is equally allocated to shred_data_cf_size - /// and shred_code_cf_size. - pub fn rocks_fifo(shred_storage_size: u64) -> ShredStorageType { - ShredStorageType::RocksFifo(BlockstoreRocksFifoOptions::new(shred_storage_size)) + /// Returns a ShredStorageType::RocksFifo, see BlockstoreRocksFifoOptions + /// for more details on how `max_shred_storage_size` is interpreted. + pub fn rocks_fifo(max_shred_storage_size: Option) -> ShredStorageType { + ShredStorageType::RocksFifo(BlockstoreRocksFifoOptions::new(max_shred_storage_size)) } /// The directory under `ledger_path` to the underlying blockstore. @@ -163,7 +162,7 @@ impl ShredStorageType { /// None will be returned if the ShredStorageType cannot be inferred. pub fn from_ledger_path( ledger_path: &Path, - fifo_shred_storage_size: u64, + max_fifo_shred_storage_size: Option, ) -> Option { let mut result: Option = None; @@ -180,7 +179,7 @@ impl ShredStorageType { { if result.is_none() { result = Some(ShredStorageType::RocksFifo( - BlockstoreRocksFifoOptions::new(fifo_shred_storage_size), + BlockstoreRocksFifoOptions::new(max_fifo_shred_storage_size), )); } else { result = None; @@ -215,10 +214,22 @@ pub struct BlockstoreRocksFifoOptions { pub const MAX_ROCKS_FIFO_SHRED_STORAGE_SIZE_BYTES: u64 = std::u64::MAX; impl BlockstoreRocksFifoOptions { - fn new(shred_storage_size: u64) -> Self { - Self { - shred_data_cf_size: shred_storage_size / 2, - shred_code_cf_size: shred_storage_size / 2, + /// Returns a BlockstoreRocksFifoOptions where the specified + /// `max_shred_storage_size` is equally split between shred_data_cf_size + /// and shred_code_cf_size. A `None` value for `max_shred_storage_size` + /// will (functionally) allow unbounded growth in these two columns. Once + /// a column's total size exceeds the configured value, the oldest file(s) + /// will be purged to get back within the limit. + fn new(max_shred_storage_size: Option) -> Self { + match max_shred_storage_size { + Some(size) => Self { + shred_data_cf_size: size / 2, + shred_code_cf_size: size / 2, + }, + None => Self { + shred_data_cf_size: MAX_ROCKS_FIFO_SHRED_STORAGE_SIZE_BYTES, + shred_code_cf_size: MAX_ROCKS_FIFO_SHRED_STORAGE_SIZE_BYTES, + }, } } diff --git a/validator/src/main.rs b/validator/src/main.rs index 18338b5f8b0c37..878046f85c96ba 100644 --- a/validator/src/main.rs +++ b/validator/src/main.rs @@ -28,7 +28,6 @@ use { solana_gossip::{cluster_info::Node, contact_info::ContactInfo}, solana_ledger::blockstore_options::{ BlockstoreCompressionType, BlockstoreRecoveryMode, LedgerColumnOptions, ShredStorageType, - MAX_ROCKS_FIFO_SHRED_STORAGE_SIZE_BYTES, }, solana_net_utils::VALIDATOR_PORT_RANGE, solana_perf::recycler::enable_recycler_warming, @@ -392,19 +391,18 @@ fn hash_validator(hash: String) -> Result<(), String> { .map_err(|e| format!("{:?}", e)) } -/// Returns the default shred storage size (include both data and coding +/// Returns the default fifo shred storage size (include both data and coding /// shreds) based on the validator config. -fn default_fifo_shred_storage_size(vc: &ValidatorConfig) -> u64 { +fn default_fifo_shred_storage_size(vc: &ValidatorConfig) -> Option { // The max shred size is around 1228 bytes. // Here we reserve a little bit more than that to give extra storage for FIFO // to prevent it from purging data that have not yet being marked as obsoleted // by LedgerCleanupService. const RESERVED_BYTES_PER_SHRED: u64 = 1500; - match vc.max_ledger_shreds { + vc.max_ledger_shreds.map(|max_ledger_shreds| { // x2 as we have data shred and coding shred. - Some(max_ledger_shreds) => max_ledger_shreds * RESERVED_BYTES_PER_SHRED * 2, - None => MAX_ROCKS_FIFO_SHRED_STORAGE_SIZE_BYTES, - } + max_ledger_shreds * RESERVED_BYTES_PER_SHRED * 2 + }) } // This function is duplicated in ledger-tool/src/main.rs... @@ -3055,11 +3053,11 @@ pub fn main() { None => ShredStorageType::rocks_fifo(default_fifo_shred_storage_size( &validator_config, )), - Some(_) => ShredStorageType::rocks_fifo(value_t_or_exit!( + Some(_) => ShredStorageType::rocks_fifo(Some(value_t_or_exit!( matches, "rocksdb_fifo_shred_storage_size", u64 - )), + ))), }, _ => panic!( "Unrecognized rocksdb-shred-compaction: {}",