refactor!(wal): factor out wal-rocksdb #1231
Conversation
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
| /// Config for wal based on RocksDB | ||
| #[derive(Debug, Clone, Deserialize, Serialize)] | ||
| #[serde(default)] | ||
| pub struct RocksDBConfig { |
There was a problem hiding this comment.
This config can be moved to wal-rocksdb while tests uses should be updated.
| } | ||
| } | ||
| } | ||
|
|
||
| /// Options for wal storage backend | ||
| #[derive(Debug, Clone, Deserialize, Serialize)] | ||
| #[serde(tag = "type")] | ||
| pub enum WalStorageConfig { |
There was a problem hiding this comment.
But this enum cannot be moved to any specific wal-impl, not suitable for wal also.
| /// Options for wal storage backend | ||
| #[derive(Debug, Clone, Deserialize, Serialize)] | ||
| #[serde(tag = "type")] | ||
| pub enum WalStorageConfig { | ||
| #[cfg(feature = "wal-rocksdb")] |
There was a problem hiding this comment.
I think this shouldn't be in feature gate, we should throw error at runtime when user configure to use rocksdb as WAL for binary built without this feature.
There was a problem hiding this comment.
But the RocksDBConfig transitively depends on rocksdb so when we build a binary without rocksdb, the RocksDB config cannot be constructed also.
There was a problem hiding this comment.
the RocksDB config cannot be constructed also.
In this case, a dummy struct could be constructed to make compile success. Something like this:
#[cfg(feature = "wal-rocksdb")]
RocksDBConfig = wal-rocksdb::config
#[cfg(not(feature = "wal-rocksdb"))]
RocksDBConfig = struct {};
| @@ -174,7 +177,10 @@ trace_metric_derive = { path = "components/trace_metric_derive" } | |||
| trace_metric_derive_tests = { path = "components/trace_metric_derive_tests" } | |||
| tonic = "0.8.1" | |||
| tokio = { version = "1.29", features = ["full"] } | |||
| wal = { path = "wal" } | |||
| wal = { path = "src/wal" } | |||
There was a problem hiding this comment.
I wonder if this is necessary to split different wal implementation to different crates, any advantages?
There was a problem hiding this comment.
It's possible we retain the code in wal crate and guard them with different feature gate. Factor out impl into dedicated crates is for:
- per Cannot build librocksdb-sys on macOS with Apple M1 chip #1222 (comment) suggested
- exclude a crate can be straightforward than conditionally opt-out some code in the same crate, but perhaps we can narrow the opt code into the same mod.
Generally, I don't have a strong preference, either should be viable.
There was a problem hiding this comment.
OK, I also don't have strong preference, keep it as it's now.
## Rationale Ref - #1231 (comment). ## Detailed Changes See the commit messages. ## Test Plan Code refactor. All existing tests should pass. --------- Signed-off-by: tison <wander4096@gmail.com>
## Rationale Part of #1231. This change itself is self-contained to merge (i.e., not half-complelted). Since that I may not have enough time to continue the next few days, I post this first for a merge to avoid code conflict. ## Detailed Changes See the commit message. ## Test Plan Refactor. Should pass all existing tests. --------- Signed-off-by: tison <wander4096@gmail.com>
|
Superseded by #1272. |
## Rationale Supersede #1231 After compile time with clean - ``` Finished dev [unoptimized + debuginfo] target(s) in 2m 18s ``` ## Detailed Changes Conditionally compile wal impls with feature flags. ## Test Plan Code refactor. All existing tests should pass. --------- Signed-off-by: tison <wander4096@gmail.com>
This is a possible impl for #1222 (comment).
But still the transitive solution introduce too much
cfgcode IMO, while otherwise the deeply coupled code cannot be easily opt out.Send for preview and demonstrate the solution direction. Perhaps we can have a better solution.