fix: Setting RocksDB as default database, making db configurable, and removing redis from deps #211
Conversation
WalkthroughThe pull request introduces a comprehensive refactoring of the database configuration system in the Prism project. The primary change is shifting from Redis to RocksDB as the default storage backend, with added flexibility to support multiple database types. A new Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
crates/cli/src/cfg.rs (4)
39-40: Consider changingnetwork_nameto aStringinstead ofOption<String>.Since
network_namehas a default value of"local", making it anOption<String>is unnecessary. Changing it to aStringsimplifies the code and ensures that the value is always present.Apply this diff to update the type:
- network_name: Option<String>, + network_name: String,And adjust the argument attribute:
- #[arg(short = 'n', long, default_value = "local")] + #[arg(short = 'n', long, default_value = "local")]
52-52: Consider changinghome_pathto aStringinstead ofOption<String>.Similar to
network_name, sincehome_pathcan have a default value, changing it to aStringensures that the value is always present, reducing the need to handleOption.Apply this diff:
- home_path: Option<String>, + home_path: String,
259-278: Implementinitialize_dbreturning a consistent type without boxing twice.Currently, the function returns
Arc<Box<dyn Database>>>, which includes unnecessary boxing. Simplify the return type toArc<dyn Database>.Apply this diff:
-pub fn initialize_db(cfg: &Config) -> Result<Arc<Box<dyn Database>>> { +pub fn initialize_db(cfg: &Config) -> Result<Arc<dyn Database>> { match &cfg.db { StorageBackend::RocksDB(cfg) => { let db = RocksDBConnection::new(cfg) .map_err(|e| GeneralError::InitializationError(e.to_string())) .context("Failed to initialize RocksDB")?; - Ok(Arc::new(Box::new(db) as Box<dyn Database>)) + Ok(Arc::new(db) as Arc<dyn Database>) } StorageBackend::InMemory => Ok(Arc::new( - Box::new(InMemoryDatabase::new()) as Box<dyn Database> + InMemoryDatabase::new() )), StorageBackend::Redis(cfg) => { let db = RedisConnection::new(cfg) .map_err(|e| GeneralError::InitializationError(e.to_string())) .context("Failed to initialize Redis")?; - Ok(Arc::new(Box::new(db) as Box<dyn Database>)) + Ok(Arc::new(db) as Arc<dyn Database>) } } }
86-86: Include default value forcelestia_start_height.Consider setting a default value for
celestia_start_heightto ensure consistent behavior when the argument is not provided.Apply this diff:
/// Height to start searching the DA layer for SNARKs on #[arg(short = 's', long)] + #[arg(default_value_t = 0)] celestia_start_height: Option<u64>,crates/storage/src/database.rs (1)
8-13: Document theStorageBackendenum and its variants.Adding documentation comments to the
StorageBackendenum and its variants improves code readability and maintainability.Apply this diff:
/// Enum representing the different storage backends available. #[derive(Debug, Clone, Eq, PartialEq, Serialize, Deserialize)] pub enum StorageBackend { + /// Uses RocksDB for persistent storage. RocksDB(crate::rocksdb::RocksDBConfig), + /// Uses an in-memory database, data is lost on shutdown. InMemory, + /// Uses Redis for storage. Redis(crate::redis::RedisConfig), }crates/tests/src/lib.rs (1)
15-18: Avoid redundant boxing of the database object.In the
setup_dbfunction, the database object is being boxed and then wrapped in anArc. This double boxing is unnecessary.Apply this diff:
use prism_storage::{ rocksdb::{RocksDBConfig, RocksDBConnection}, Database, }; use rand::{rngs::StdRng, Rng, SeedableRng}; fn setup_db() -> Arc<dyn Database> { let temp_dir = TempDir::new().unwrap(); let cfg = RocksDBConfig::new(temp_dir.path().to_str().unwrap()); - let db = RocksDBConnection::new(&cfg).unwrap(); - Arc::new(Box::new(db) as Box<dyn Database>) + let db: Arc<dyn Database> = Arc::new(RocksDBConnection::new(&cfg).unwrap()); + db }crates/storage/src/rocksdb.rs (2)
29-35: Consider adding path validation.The constructor accepts any string as a path without validation. Consider adding basic path validation to ensure the path is valid and accessible.
impl RocksDBConfig { pub fn new(path: &str) -> Self { + // Validate path + let path = std::path::Path::new(path); + if !path.parent().map_or(true, |p| p.exists()) { + std::fs::create_dir_all(path.parent().unwrap()).expect("Failed to create directory"); + } Self { - path: path.to_string(), + path: path.to_str().unwrap().to_string(), } } }
44-45: Consider adding database options configuration.The implementation uses default RocksDB options. Consider allowing customization of database options through the config struct for better performance tuning.
crates/cli/src/main.rs (1)
104-107: Consider structured logging for errors.The error logging could be enhanced with structured fields to provide more context about the failure.
- error!("error initializing prover: {}", e); + error!(error = %e, component = "prover", "Failed to initialize prover");Also applies to: 159-162
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
crates/cli/src/cfg.rs(9 hunks)crates/cli/src/main.rs(5 hunks)crates/storage/src/database.rs(1 hunks)crates/storage/src/redis.rs(1 hunks)crates/storage/src/rocksdb.rs(2 hunks)crates/tests/src/lib.rs(2 hunks)justfile(0 hunks)
💤 Files with no reviewable changes (1)
- justfile
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: integration-test
- GitHub Check: unused dependencies
- GitHub Check: clippy
- GitHub Check: unit-test
- GitHub Check: build-and-push-image
🔇 Additional comments (9)
crates/cli/src/cfg.rs (3)
124-124: Ensure default configuration aligns with user expectations.When creating a default
Configwithwith_home, confirm that settingRocksDBas the defaultStorageBackendis appropriate, especially if the user hasn't explicitly configured it.Consider whether
InMemorymight be a safer default to prevent unexpected disk usage.
225-232: Handle missing database configuration parameters gracefully.When
db_typeisRedisorRocksDB, ensure thatredis_urlorrocksdb_pathare provided. Currently, missing parameters might lead to defaults that could cause unexpected behavior.Run the following script to check if appropriate error handling is in place:
185-191: Simplify path construction and handle potential errors.Ensure that the constructed home path is valid and handle cases where the home directory might not be available.
Modify the code as follows:
fn get_prism_home(args: &CommandArgs) -> Result<String> { - let network_name = args.network_name.clone().unwrap_or_else(|| "custom".to_string()); - args.home_path - .clone() - .or_else(|| { - home_dir().map(|path| format!("{}/.prism/{}/", path.to_string_lossy(), network_name)) - }) + let network_name = args.network_name.clone(); + let home = args.home_path.clone().or_else(|| { + home_dir().map(|path| path.to_string_lossy().to_string()) + }); + let prism_home = format!("{}/.prism/{}/", home.unwrap_or_default(), network_name); .ok_or_else(|| { GeneralError::MissingArgumentError("could not determine config path".to_string()).into() }) }Confirm that the application handles cases where the home directory is not available.
crates/storage/src/database.rs (1)
8-13: Consider consistent naming for storage backends.Ensure that the naming of storage backends is consistent across the codebase. For example, use
RocksDbinstead ofRocksDBfor consistency.Apply this diff:
pub enum StorageBackend { - RocksDB(crate::rocksdb::RocksDBConfig), + RocksDb(crate::rocksdb::RocksDBConfig), InMemory, - Redis(crate::redis::RedisConfig), + RedisDb(crate::redis::RedisConfig), }Confirm that renaming does not break any references elsewhere in the codebase.
crates/tests/src/lib.rs (1)
30-31:⚠️ Potential issueHandle potential
Nonevalue fromtemp_dir.path().to_str().The
to_str()method can returnNoneif the path is not valid UTF-8. Handle this case to prevent potential errors.Apply this diff:
let temp_dir = TempDir::new().unwrap(); -let cfg = RocksDBConfig::new(temp_dir.path().to_str().unwrap()); +let temp_path = temp_dir.path().to_str().expect("Temp path is not valid UTF-8"); +let cfg = RocksDBConfig::new(temp_path);Ensure that the test fails gracefully if the path is invalid.
crates/storage/src/redis.rs (2)
26-26: LGTM! AddingPartialEqandEqtraits.The addition of these traits allows for equality comparisons of
RedisConfiginstances, which is good practice for configuration types.
Line range hint
1-200: Verify if this file should be removed.According to the PR objectives, Redis is being removed from dependencies and RocksDB is being set as the default database. However, this file still contains the Redis implementation.
crates/storage/src/rocksdb.rs (1)
24-27: LGTM! Well-structured config type.The
RocksDBConfigstruct is properly defined with appropriate derive attributes for serialization, comparison, and debugging.crates/cli/src/main.rs (1)
69-70: LGTM! Consistent database initialization.The database initialization is implemented consistently across both Prover and FullNode commands with proper error handling.
Also applies to: 117-118
| #[derive(Args, Deserialize, Clone, Debug)] | ||
| pub struct DatabaseArgs { | ||
| #[arg(long, value_enum)] | ||
| db_type: Option<DBValues>, |
|
|
||
| /// Path to the RocksDB database, used when `db_type` is `rocks-db` | ||
| #[arg(long)] | ||
| rocksdb_path: Option<String>, |
There was a problem hiding this comment.
I propose to have the default path <homePath>/data instead of <homePath>.
Currently, all the data files and config are in the same folder.
|
|
||
| /// Connection string to Redis, used when `db_type` is `redis` | ||
| #[arg(long)] | ||
| redis_url: Option<String>, |
There was a problem hiding this comment.
Improve error handling:
If db_type is redis throw an error saying the user needs to set redis_url.
| @@ -43,7 +49,10 @@ pub struct CommandArgs { | |||
| verifying_key_algorithm: Option<String>, | |||
|
|
|||
| #[arg(long)] | |||
| config_path: Option<String>, | |||
| home_path: Option<String>, | |||
--network-name(default:local). Default prism home is now{HOME}/.prism/{NETWORK_NAME}--db-type(possible values:rocks-db,in-memory,redis). rocks-db requires--rocksdb-path, redis requires--redis-urlCloses #196
Summary by CodeRabbit
Release Notes
New Features
Changes
Improvements