feat(storage): add Batch trait abstraction for atomic write operations #231
Conversation
📝 WalkthroughWalkthroughAdds a public Batch trait with two implementations (RocksBatch for standalone, BinlogBatch placeholder for cluster), re-exports them, and migrates Redis storage modules to create and use batches via Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Redis
participant Batch
participant RocksDB
Client->>Redis: write command (e.g., HSET/ZADD/LPUSH)
Redis->>Redis: prepare encoded keys & metadata
Redis->>Batch: create_batch()
Redis->>Batch: put/delete per ColumnFamilyIndex
Redis->>Batch: commit()
alt RocksBatch (standalone)
Batch->>RocksDB: db.write_opt(WriteBatch, WriteOptions)
RocksDB-->>Batch: write result
else BinlogBatch (cluster placeholder)
Batch-->>Redis: (TODO: Raft/binlog integration)
end
Batch-->>Redis: commit result
Redis-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/storage/src/redis_sets.rs (1)
2182-2252:SDIFFSTORE/*storeoperations are no longer atomic (two commits).At Line 2239-2247 (and similarly Line 2362-2375), the code commits a “clear destination” batch, then calls
self.sadd(...)which commits again. That creates an observable intermediate state (destination empty/partially written) and breaks the typical atomic semantics expected from*STORE.Suggestion: build one batch that (1) deletes old destination meta + members and (2) writes the new meta + members, then commit once. If you want to reuse
sadd, consider adding an internal helper likesadd_into_batch(&mut dyn Batch, ...)that doesn’t commit.Also applies to: 2309-2377
🤖 Fix all issues with AI agents
In @src/storage/src/batch.rs:
- Around line 47-79: The Batch trait currently exposes put/delete as infallible
and RocksBatch implementation uses assert!/expect! which can panic; change the
trait signatures for put and delete to return Result<(), ErrorType> (or make
them return Result<()> using the crate's common error type), update
RocksBatch::{put, delete} to validate column family lookup without assert/expect
and return Err(...) on invalid CF or other failures, and propagate/store any
internal errors so that commit(self: Box<Self>) returns those errors instead of
panicking; update all call sites to handle the new Result return values and
ensure commit still returns Result<()> with any accumulated error.
- Around line 38-45: Run rustfmt on the new module to resolve the formatting
warning: run `cargo fmt` (or apply rustfmt) for src/storage/src/batch.rs so the
use/import block is properly ordered and spaced (std::sync::Arc;
rocksdb::{BoundColumnFamily, WriteBatch, WriteOptions}; snafu::ResultExt;
crate::error::{Result, RocksSnafu}; crate::ColumnFamilyIndex; engine::Engine).
Ensure no extra blank lines or misaligned imports remain so CI formatting check
passes.
- Around line 166-224: BinlogBatch::commit currently returns Ok(()) while doing
nothing; change it to return an explicit not-implemented error (e.g.,
Err(Error::unimplemented or a suitable crate::error::Error variant) from the
commit method) so callers (including create_batch when it may return
BinlogBatch) cannot acknowledge writes that aren’t persisted; update the commit
implementation in the BinlogBatch impl to construct and return that explicit
error and keep the method body otherwise unchanged until Raft append logic is
implemented.
🧹 Nitpick comments (5)
src/storage/src/redis_strings.rs (1)
2107-2151: Potentially unbounded in-memory key collection forDEL/FLUSHDBpaths.At Line 2110-2151 and 2226-2262, keys are collected into a
Vecand then deleted via one batch commit. For large DBs this can spike memory and produce very largeWriteBatches.Consider chunking: delete/commit every N keys (or stream deletes directly into a batch and commit when
batch.count()reaches a threshold).Also applies to: 2226-2262
src/storage/src/redis_lists.rs (2)
319-349:lpop/rpopnow apply deletes + meta update in one batch — good.The new
keys_to_deletecollection and single batch commit (Line 341-349, 412-420, 483-491) is consistent.Also applies to: 390-420, 461-491
754-806: Batch delete/put loops are correct; consider writing directly into batch to reduce allocations.Several paths build
Vec<Vec<u8>>andVec<(Vec<u8>, Vec<u8>)>first (e.g., Line 754-806, 887-937, 1032-1073). Where feasible, you can push operations directly intobatchas you compute them to avoid duplicating key/value buffers.Also applies to: 887-937, 1032-1073
src/storage/src/redis_hashes.rs (2)
112-118: Formatting issue flagged by CI.The batch operations are correct, but the CI cargo fmt check indicates formatting differences. Run
cargo fmtto fix.🧹 Run formatter
cargo fmt --all
289-303: Helper closure pattern works but creates some duplication.The
create_new_hashclosure is duplicated acrosshset,hmset,hsetnx,hincrby, andhincrbyfloat. While the closures capture method-specificbase_meta_key, consider extracting to a shared method that takesbase_meta_keyas a parameter to reduce duplication. This is a nice-to-have refactor for future.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/storage/src/batch.rssrc/storage/src/lib.rssrc/storage/src/redis.rssrc/storage/src/redis_hashes.rssrc/storage/src/redis_lists.rssrc/storage/src/redis_sets.rssrc/storage/src/redis_strings.rssrc/storage/src/redis_zsets.rs
🧰 Additional context used
🧬 Code graph analysis (5)
src/storage/src/redis_sets.rs (1)
src/storage/src/batch.rs (2)
new(102-114)new(186-188)
src/storage/src/redis_strings.rs (1)
src/storage/src/batch.rs (2)
new(102-114)new(186-188)
src/storage/src/redis_lists.rs (2)
src/storage/src/batch.rs (1)
new(102-114)src/engine/src/rocksdb_engine.rs (1)
db(38-40)
src/storage/src/redis_hashes.rs (1)
src/storage/src/batch.rs (2)
new(102-114)new(186-188)
src/storage/src/redis_zsets.rs (1)
src/storage/src/batch.rs (2)
new(102-114)new(186-188)
🪛 GitHub Check: cargo fmt (ubuntu-latest)
src/storage/src/batch.rs
[warning] 40-40:
Diff in /home/runner/work/kiwi/kiwi/src/storage/src/batch.rs
src/storage/src/redis_hashes.rs
[warning] 113-113:
Diff in /home/runner/work/kiwi/kiwi/src/storage/src/redis_hashes.rs
[warning] 297-297:
Diff in /home/runner/work/kiwi/kiwi/src/storage/src/redis_hashes.rs
[warning] 622-622:
Diff in /home/runner/work/kiwi/kiwi/src/storage/src/redis_hashes.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: cargo clippy (macos-latest)
- GitHub Check: cargo clippy (windows-latest)
- GitHub Check: build and test (macos-latest)
- GitHub Check: build and test (windows-latest)
- GitHub Check: build and test (ubuntu-latest)
- GitHub Check: cargo clippy (ubuntu-latest)
- GitHub Check: lint
- GitHub Check: test-macos
- GitHub Check: test-windows
- GitHub Check: test
🔇 Additional comments (19)
src/storage/src/lib.rs (1)
42-43: Batch module wiring + re-export looks good.
mod batch;(Line 42) andpub use batch::{Batch, BinlogBatch, RocksBatch};(Line 69) are consistent with the new abstraction.Also applies to: 69-70
src/storage/src/redis_strings.rs (1)
318-325: Batch refactor for string writes is consistent and improves atomicity.The
create_batch() -> put/delete -> commit()pattern applied across setters (e.g., Line 581-588, 635-642, 1063-1079) is consistent and keeps write paths uniform.Also applies to: 389-396, 581-588, 635-642, 698-705, 759-766, 831-838, 1063-1079
src/storage/src/redis_sets.rs (1)
151-166: Batch usage in set mutators (sadd/srem/spop/smove) looks coherent.The conversions to
create_batch()withMetaCF+SetsDataCFoperations and a singlecommit()(e.g., Line 151-166, 494-512, 783-813) match the intended abstraction.Also applies to: 494-512, 627-647, 783-813
src/storage/src/redis_lists.rs (1)
105-108: Listpush_core()batching approach is clean and keeps meta+data updates in one commit.Collecting deletes/puts and committing them together (Line 236-246) aligns well with the new Batch abstraction and should preserve atomicity for each list mutation.
Also applies to: 236-246
src/storage/src/redis_zsets.rs (9)
30-30: Import cleanup looks good.The removal of
WriteBatchfrom the import is consistent with the migration to the new batch abstraction.
99-166: Batch abstraction correctly applied for existing zset updates.The migration from direct
WriteBatchto the new batch abstraction maintains atomicity - all score key deletions, member/score insertions, and meta updates are committed together.
169-189: New zset creation path correctly uses batch abstraction.The batch lifecycle (create → put → commit) is properly implemented for the new zset creation path.
357-376: Score increment batch operations are correct.The batch properly handles the atomic update of: delete old score key → put new member value → put new score key → commit.
791-843: Member removal batch operations are well-structured.The batch correctly handles multi-member deletion with proper meta update logic (delete meta if count reaches zero, otherwise update count). Statistics update is appropriately placed after successful commit.
1510-1528: Lex range removal uses correct batch pattern.The collect-then-batch approach is efficient, and all deletions plus meta updates are atomic.
1629-1647: Rank range removal batch operations are correct.Consistent pattern with other range removal methods.
1728-1746: Score range removal batch operations are correct.Consistent and correct batch usage pattern.
1177-1240: Two-phase batch approach for store operations is acceptable.The destination cleanup uses one batch (lines 1197-1227), then results are added via
zaddwhich uses its own batch. This maintains atomicity within each phase. The lock is correctly released before callingzaddto prevent deadlock.Note: In edge cases where the first batch commits but
zaddfails, the destination would be cleared but not populated. This matches Redis behavior where a failure partway through leaves partial state.src/storage/src/redis_hashes.rs (6)
23-23: Import cleanup is consistent with batch migration.Removed
WriteBatchimport as expected for the batch abstraction migration.
335-339: Batch operations for stale hash reinit are correct.The batch properly updates both meta and data in a single atomic commit.
696-704: Conditional meta update is correct but could be simplified.The meta is only updated when
new_fields_count > 0, which is correct. However, the batch is always created and committed even when no new fields are added (just updates to existing fields). This is fine for correctness but creates a batch commit for updates-only scenarios.
903-914: Integer overflow protection and batch operation are correct.The
checked_addprevents overflow, and the batch commit properly persists the incremented value.
1032-1046: Float overflow protection and batch operation are correct.The
is_finite()check properly guards against NaN/Infinity results before committing.
774-778: HSETNX batch operations are correct.Atomic commit of meta and data for stale hash reinitialization.
Tsukikage7
force-pushed
the
feature/support-batch
branch
from
0531ae0 to
b86f898
Compare
Tsukikage7
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/storage/src/redis_sets.rs (1)
2206-2276: Atomicity regression risk in*STORE: destination clear and repopulate happen in multiple commits.
sdiffstore()(andclear_and_store_set()used bysinterstore/sunionstore) currently:
- deletes existing destination keys and commits
- calls
sadd()which commits againThis opens race windows (and intermediate visibility) and can violate “single command is atomic” expectations, especially since reads typically don’t take locks.
Suggested fix: do clear + repopulate in one batch commit
- // Write the batch to clear destination first - let mut batch = self.create_batch()?; - ... - batch.commit()?; - - // Now add the new members - let added = self.sadd(destination, &member_refs)?; - Ok(added) + // Clear + write new meta + write new members in ONE batch + let mut batch = self.create_batch()?; + for key_to_del in &keys_to_delete { + batch.delete(ColumnFamilyIndex::SetsDataCF, key_to_del)?; + } + if should_delete_meta { + batch.delete(ColumnFamilyIndex::MetaCF, &dest_base_meta_key)?; + } + + // Build and write destination meta + members here (avoid calling sadd()) + // - create new BaseMetaValue(DataType::Set) with count=members.len() + // - get version from ParsedSetsMetaValue::initial_meta_value() + // - for each member: MemberDataKey::new(destination, version, member).encode() and batch.put(...) + + batch.commit()?; + Ok(members.len() as i32)Also applies to: 2333-2401
🤖 Fix all issues with AI agents
In @src/storage/src/redis.rs:
- Around line 320-360: The code relies on positional indexing of self.handles
(via get_cf_handle/ColumnFamilyIndex) but initialization used filter() which can
remove missing CFs and shift positions, breaking the mapping; change the
implementation to use a stable name-based mapping instead: during DB open
populate a fixed-size Vec<Option<Arc<rocksdb::BoundColumnFamily<'_>>>> or a
HashMap<ColumnFamilyIndex, Arc<...>> keyed by ColumnFamilyIndex (or CF name) so
missing CFs are represented as None rather than removed, update get_cf_handle to
return the handle by lookup (not by index into a filtered Vec), and ensure
create_batch collects handles by calling the new get_cf_handle for each
ColumnFamilyIndex (MetaCF, HashesDataCF, SetsDataCF, ListsDataCF, ZsetsDataCF,
ZsetsScoreCF) so ordering/invariants are preserved.
🧹 Nitpick comments (4)
src/storage/src/batch.rs (1)
95-199: RocksBatch CF index validation is good; consider tightening diagnostics + avoiding repeated custom Location building.
- The “max” in the bounds error reads like an index but is currently
len; considerlen.saturating_sub(1).- You can simplify the error location to
snafu::location!()(and keep the message).src/storage/src/redis_lists.rs (1)
718-806: Nice consolidation to single-commit mutations; consider avoiding pre-staging when not needed.Several paths build
Vecof keys/puts first and then replay intobatch.*. If borrow/lifetime constraints permit, writing directly intobatchas you compute keys would save memory and copies on large lists.Also applies to: 887-937, 1032-1073
src/storage/src/redis_strings.rs (1)
2110-2151: Potential memory spike: del_key/flush_db collect all keys before deleting.For large datasets,
keys_to_deletecan become huge. Consider chunked deletion (e.g., commit every N ops andbatch.clear()), or implement a purpose-built “delete range/prefix” API in the engine later.Also applies to: 2226-2261
src/storage/src/redis_hashes.rs (1)
294-314: Consider extracting repeated helper pattern.The
create_new_hashhelper closure appears with minor variations across multiple methods (also inhsetnxat lines 780-800,hincrbyat lines 905-926, andhincrbyfloatat lines 1061-1082). While the current implementation works correctly and maintains atomicity, extracting this to a shared generic helper method could reduce duplication.Example consolidation approach
A shared method could accept a value encoder:
fn create_new_hash_with_field<F>( &self, key: &[u8], base_meta_key: &[u8], field: &[u8], encode_value: F, ) -> Result<()> where F: FnOnce() -> Vec<u8>, { let mut hashes_meta = HashesMetaValue::new(Bytes::copy_from_slice(&1u64.to_le_bytes())); hashes_meta.inner.data_type = DataType::Hash; let version = hashes_meta.update_version(); let data_key = MemberDataKey::new(key, version, field); let data_value = BaseDataValue::new(encode_value()); let mut batch = self.create_batch()?; batch.put(ColumnFamilyIndex::MetaCF, base_meta_key, &hashes_meta.encode())?; batch.put(ColumnFamilyIndex::HashesDataCF, &data_key.encode()?, &data_value.encode())?; batch.commit() }Then call sites would become:
create_new_hash_with_field(key, &base_meta_key, field, || value.to_string().into_bytes())?
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/storage/src/batch.rssrc/storage/src/lib.rssrc/storage/src/redis.rssrc/storage/src/redis_hashes.rssrc/storage/src/redis_lists.rssrc/storage/src/redis_sets.rssrc/storage/src/redis_strings.rssrc/storage/src/redis_zsets.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/storage/src/lib.rs
- src/storage/src/redis_zsets.rs
🧰 Additional context used
🧬 Code graph analysis (5)
src/storage/src/redis_sets.rs (1)
src/storage/src/batch.rs (2)
new(113-125)new(221-223)
src/storage/src/redis_lists.rs (2)
src/storage/src/batch.rs (1)
new(113-125)src/engine/src/rocksdb_engine.rs (1)
db(38-40)
src/storage/src/redis_strings.rs (1)
src/storage/src/batch.rs (2)
new(113-125)new(221-223)
src/storage/src/redis_hashes.rs (1)
src/storage/src/batch.rs (2)
new(113-125)new(221-223)
src/storage/src/batch.rs (2)
src/storage/src/redis.rs (1)
new(115-149)src/engine/src/rocksdb_engine.rs (1)
db(38-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-windows
- GitHub Check: cargo clippy (macos-latest)
🔇 Additional comments (13)
src/storage/src/batch.rs (2)
47-90: Batch trait shape looks good for “commit consumes batch” flow.
commit(self: Box<Self>)aligns withRedis::create_batch() -> Box<dyn Batch>and avoids post-commit reuse footguns.
201-265: BinlogBatch correctly fails commit to avoid silent data loss.Returning an error from
commit()is the right default until Raft integration lands.src/storage/src/redis_lists.rs (2)
105-246: push_core batching integration looks consistent (data CF ops + meta update in one commit).Order of operations (deletes, puts, meta put, commit) is coherent and keeps the metadata update in the same atomic commit.
317-349: Pop paths correctly batch element deletions with metadata update.Keeping meta written even when empty matches the comment/intent and avoids breaking lpushx/rpushx flows.
Also applies to: 388-420, 459-491
src/storage/src/redis_strings.rs (1)
318-325: Batch-based writes are consistently applied across string mutations.These conversions preserve “single logical operation → single commit” and fit the new Batch abstraction well.
Also applies to: 389-396, 581-588, 635-642, 698-705, 759-766, 831-838, 1063-1079, 1137-1153, 1212-1219, 1284-1291, 1836-1884, 1938-1945, 1979-1986
src/storage/src/redis_sets.rs (1)
151-169: Core set mutations migrated cleanly to Batch (single commit).These paths keep metadata + member updates together and look correct.
Also applies to: 472-520, 634-659, 795-837
src/storage/src/redis_hashes.rs (7)
86-122: LGTM! Efficient collect-then-batch pattern.The refactoring correctly implements the batch abstraction with a two-phase approach: first collecting keys that need deletion (lines 86-99), then performing all operations in a single atomic batch (lines 112-121). This ensures consistency between the metadata count and actual deletions.
346-407: LGTM! Batch operations correctly handle all branches.The batch abstraction is properly applied across all three logical branches:
- Stale/empty hash initialization (lines 346-357)
- Existing field updates (lines 376-382)
- New field additions (lines 396-407)
Each branch maintains atomicity guarantees and correctly coordinates metadata updates with data changes.
648-664: LGTM! Efficient multi-field batch pattern.The helper correctly consolidates all field-value pairs into a single batch operation, writing the metadata once followed by all data entries. This is more efficient than per-field batches and maintains atomicity for the entire multi-set operation.
716-755: LGTM! Two-phase batch strategy for existing hashes.The collect-then-batch pattern (lines 716-730 collect, lines 743-754 batch) correctly:
- Checks field existence before batching to determine new field count
- Accumulates all writes before committing
- Conditionally updates metadata only when new fields are added
- Commits everything atomically in a single batch
832-876: LGTM! Batch operations correctly implement set-if-not-exists semantics.Both the stale hash reinitialization (lines 832-843) and new field addition (lines 865-876) paths correctly use atomic batch operations to coordinate metadata and data updates.
959-1032: LGTM! Atomic increment operations with proper overflow handling.The batch-based implementation correctly handles:
- Integer overflow detection before committing (line 993)
- Atomic updates for existing fields (lines 1000-1006)
- Coordinated meta+data writes for new fields (lines 1021-1032)
All branches maintain transactional consistency through the batch abstraction.
1115-1191: LGTM! Atomic float increment with proper validation.The implementation correctly:
- Validates float results are finite before committing (lines 1148-1154)
- Uses atomic batch updates for existing fields (lines 1159-1165)
- Coordinates meta+data writes for new fields (lines 1180-1191)
The batch abstraction maintains consistency across all paths.
Tsukikage7
force-pushed
the
feature/support-batch
branch
2 times, most recently
from
5b92f55 to
0fd2c70
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
src/storage/src/redis_zsets.rs (2)
59-65: Fix score equality check inzadd(currently compares the wrong value).This condition is wrong: it checks
abs(sm.score)instead ofabs(existing_score - sm.score), so it will “skip” updates incorrectly whensm.scoreis near 0.Proposed fix
- if (existing_score, sm.score).1.abs() < f64::EPSILON { + if (existing_score - sm.score).abs() < f64::EPSILON { // Score is the same, skip continue; } else {Also applies to: 99-180, 188-213
1234-1265:zset_store_operation: delete+recreate is not atomic vs concurrent writers (lock gap + two commits).You delete destination in one commit, then (after releasing the lock) call
zadd()which does a second commit. A concurrent writer can observe/modify the destination in-between. Consider an internal “zadd without lock” (called while holding the destination lock) or building the destination contents in the same batch/commit.src/storage/src/redis_sets.rs (3)
472-520: Usesaturating_subwhen decrementing set counts.If metadata is inconsistent/corrupted,
set_meta.count() - removed_countcan underflow. Other modules already usesaturating_sub.Proposed fix
- let new_count = set_meta.count() - removed_count as u64; + let new_count = set_meta.count().saturating_sub(removed_count as u64);
2206-2271:sdiffstore: destination is cleared without a destination lock (race vs concurrent ops).You clear destination keys/meta via batch without holding the destination key lock, then later
sadd()takes the lock. This breaks the per-key lock contract used elsewhere (read-modify-write ops won’t synchronize with the clear).Sketch fix (lock destination during clear phase)
pub fn sdiffstore(&self, destination: &[u8], keys: &[&[u8]]) -> Result<i32> { + let dest_str = String::from_utf8_lossy(destination).to_string(); + let _dest_lock = ScopeRecordLock::new(self.lock_mgr.as_ref(), &dest_str); ... }
2333-2401:clear_and_store_set: Fix self-deadlock caused by nested lock acquisition
clear_and_store_setholds aScopeRecordLockon the destination key, then callsself.sadd(destination, &member_refs)which attempts to acquire the same lock again. Sinceparking_lot::Mutexis not re-entrant, this causes a self-deadlock. Refactor to either:
- Extract the core
saddlogic into a separate internal method that does not acquire the lock, then call it from bothsaddandclear_and_store_setwhile holding the lock once- Perform the
saddoperations directly withinclear_and_store_setwithout callingsaddsrc/storage/src/redis_strings.rs (3)
1106-1155:msetnxexistence check is not type-agnostic (can overwrite live non-string keys).The loop checks only
BaseKey(string) entries in MetaCF. If a key exists as hash/set/zset/list (stored underBaseMetaKey),msetnxcan incorrectly proceed and overwrite it—contradicting the comment “any live key (any type) blocks the batch”.One possible fix: reuse `get_key_type` for existence
pub fn msetnx(&self, kvs: &[(Vec<u8>, Vec<u8>)]) -> Result<bool> { - let db = self.db.as_ref().context(OptionNoneSnafu { - message: "db is not initialized".to_string(), - })?; - - let _cf = self - .get_cf_handle(ColumnFamilyIndex::MetaCF) - .context(OptionNoneSnafu { - message: "cf is not initialized".to_string(), - })?; - - // Check if any key exists and is not expired - for (key, _) in kvs { - let string_key = BaseKey::new(key); - - match db - .get_opt(&string_key.encode()?, &self.read_options) - .context(RocksSnafu)? - { - Some(val) => { - let string_value = ParsedStringsValue::new(&val[..])?; - if !string_value.is_stale() { - return Ok(false); - } - } - None => {} - } - } + // Check if any *live* key exists (any type blocks MSETNX, Redis-compatible). + for (key, _) in kvs { + if self.get_key_type(key).is_ok() { + return Ok(false); + } + }
2226-2261:flush_dbaccumulates unbounded batches on large databases; implement chunked deletes (commit every N keys) or adddelete_rangesupport to the Batch trait.The current implementation iterates all keys across multiple column families, collects them into a single
Vec, then adds all delete operations to a singleWriteBatchbefore committing. On large databases, this causes:
- Unnecessary memory allocation for the entire key vector
- Single massive write batch with thousands/millions of operations
- Increased WAL overhead and compaction pressure
RocksDB 0.23.0 provides
delete_range_cf()for efficient range deletes, but it's not exposed by theBatchtrait. Chunking deletes into smaller batches (commit every 10k–100k keys) would be a low-risk fix; alternatively, extend theBatchtrait to supportdelete_range().
2110-2151: Fix prefix-scan logic and limit batch accumulation indel_key.The current implementation has two issues:
Incorrect prefix matching:
BaseKey::encode()includes the 16-bytereserve2suffix, but composite keys (HashesDataCF, SetsDataCF, etc.) storeversionbytes immediately after the encoded user key. The checkk.starts_with(&encoded)will not match these keys because the byte sequence after the user key is[version][data], not[reserve2]. Useencode_seek_key()or a prefix withoutreserve2instead (seeredis_hashes.rs:225for the correct pattern).Unbounded memory accumulation: Collecting all matched keys into
keys_to_deletebefore deletion can exhaust memory for large composite types. Build and commit the batch incrementally instead (seeredis_multi.rs:462-468for the pattern).src/storage/src/redis_hashes.rs (1)
86-122: Fix clippy/lint failures: avoid&temp_vec()/ needless borrow-to-deref inbatch.put(...).CI is failing with “this expression creates a reference which is immediately dereferenced” at Line 119/350/400/698/751/836/869/963/1025/1119. The typical trigger here is passing
&some_vec(or&some_fn_returning_vec()) where&[u8]is expected. Bind encoded values and pass slices.Concrete fix pattern (apply to all reported lines)
- batch.put( - ColumnFamilyIndex::MetaCF, - &base_meta_key, - &meta_val.encoded(), - )?; + let encoded_meta = meta_val.encoded(); + batch.put( + ColumnFamilyIndex::MetaCF, + base_meta_key.as_slice(), + encoded_meta.as_slice(), + )?;- batch.put( - ColumnFamilyIndex::MetaCF, - &base_meta_key, - &hashes_meta.encode(), - )?; + let encoded_meta = hashes_meta.encode(); + batch.put( + ColumnFamilyIndex::MetaCF, + base_meta_key.as_slice(), + encoded_meta.as_slice(), + )?;- batch.put( - ColumnFamilyIndex::HashesDataCF, - &data_key.encode()?, - &data_value.encode(), - )?; + let encoded_key = data_key.encode()?; + let encoded_val = data_value.encode(); + batch.put( + ColumnFamilyIndex::HashesDataCF, + encoded_key.as_slice(), + encoded_val.as_slice(), + )?;Also applies to: 293-314, 346-358, 376-383, 395-408, 637-755, 779-877, 904-1033, 1060-1192
src/storage/src/redis_lists.rs (1)
1080-1154:rpoplpushis not crash-atomic (two commits); consider a single batch commit spanning both keys.Even with both key locks held,
rpop_internalcommits first andpush_corecommits second; a crash between them can lose/misplace an element. With the new Batch abstraction, it should be possible to accumulate deletes/puts for both source+destination into one batch and commit once.Also applies to: 364-429, 43-255
🤖 Fix all issues with AI agents
In @src/storage/src/batch.rs:
- Around line 56-90: The out-of-bounds error messages for ColumnFamilyIndex use
self.cf_handles.len() as the "max index" but the true maximum valid index is
self.cf_handles.len() - 1; update the error construction in the code paths that
validate cf_idx (referencing cf_handles and the Redis::create_batch() usage) to
report the maximum as self.cf_handles.len().saturating_sub(1) (or handle empty
cf_handles explicitly) so the message shows the correct highest valid index
instead of the length.
- Around line 201-264: The code relies on commented-out selection logic and can
accidentally return a BinlogBatch; add an explicit guard in Redis and
create_batch(): add a bool field like `cluster_mode` to the `Redis` struct (or
use a feature flag), then update `create_batch()` to only construct/return
`BinlogBatch` when `cluster_mode` is true and the required Raft wiring (e.g.,
append log callback) is present; otherwise always return `RocksBatch` and if
`cluster_mode` is true but append-log wiring is missing, return an immediate
error or panic rather than creating a BinlogBatch (leave `BinlogBatch::commit()`
intentional error as a secondary safeguard).
In @src/storage/src/redis_lists.rs:
- Around line 341-349: The code inconsistently handles empty lists:
lpop/rpop/rpop_internal currently preserve list metadata when the list becomes
empty while ltrim/lrem delete it, causing differing "key exists" and
lpushx/rpushx behavior; make this consistent by changing the ltrim and lrem code
paths to preserve metadata instead of deleting the MetaCF entry when a list
becomes empty (or alternatively change lpop/rpop to delete if you prefer that
semantics), updating the batch operations in the affected functions (ltrim,
lrem, and their internal helpers) so they write the parsed_meta.value() to
ColumnFamilyIndex::MetaCF rather than issuing batch.delete, and ensure the same
approach is applied to all referenced code paths (including the rpop_internal
and any other empty-list branches) so key-exists semantics match across
operations.
In @src/storage/src/redis_strings.rs:
- Around line 1063-1079: The mset implementation currently batches puts and
commits without acquiring per-key locks, which can race with ops that rely on
ScopeRecordLock (e.g., incr_decr); modify the mset function to first collect and
sort the keys (use BaseKey::new(key).encode() or the key strings), acquire a
ScopeRecordLock for each key in that sorted order (holding all locks), then
perform create_batch()/batch.put(...) for each kv and finally batch.commit(),
releasing locks after commit; ensure lock acquisition is exception-safe (release
on error) and reference ScopeRecordLock, mset, create_batch, batch.put, and
batch.commit in your change.
🧹 Nitpick comments (3)
src/raft/src/network.rs (1)
386-397: Extract duplicated message type computation into a helper method.The match expression that computes
msg_typeis duplicated betweenadd_authenticationandverify_authentication. This violates the DRY principle and creates a maintenance risk—if the message type representation needs to change, both locations must be updated consistently.♻️ Proposed refactor to eliminate duplication
Add a private helper method to
MessageEnvelope:impl MessageEnvelope { /// Get the message type representation for HMAC computation fn message_type_for_hmac(&self) -> String { match &self.message { RaftMessage::AppendEntries(_) => "AppendEntries".to_string(), RaftMessage::AppendEntriesResponse(_) => "AppendEntriesResponse".to_string(), RaftMessage::Vote(_) => "Vote".to_string(), RaftMessage::VoteResponse(_) => "VoteResponse".to_string(), RaftMessage::InstallSnapshot(_) => "InstallSnapshot".to_string(), RaftMessage::InstallSnapshotResponse(_) => "InstallSnapshotResponse".to_string(), RaftMessage::Heartbeat { from, term } => format!("Heartbeat:{}:{}", from, term), RaftMessage::HeartbeatResponse { from, success } => { format!("HeartbeatResponse:{}:{}", from, success) } } } }Then update both methods to use the helper:
pub fn add_authentication(&mut self, auth: &NodeAuth) -> RaftResult<()> { - // Use a simplified message representation for HMAC - let msg_type = match &self.message { - RaftMessage::AppendEntries(_) => "AppendEntries".to_string(), - RaftMessage::AppendEntriesResponse(_) => "AppendEntriesResponse".to_string(), - RaftMessage::Vote(_) => "Vote".to_string(), - RaftMessage::VoteResponse(_) => "VoteResponse".to_string(), - RaftMessage::InstallSnapshot(_) => "InstallSnapshot".to_string(), - RaftMessage::InstallSnapshotResponse(_) => "InstallSnapshotResponse".to_string(), - RaftMessage::Heartbeat { from, term } => format!("Heartbeat:{}:{}", from, term), - RaftMessage::HeartbeatResponse { from, success } => { - format!("HeartbeatResponse:{}:{}", from, success) - } - }; + let msg_type = self.message_type_for_hmac(); let data_for_hmac = format!( "{}:{}:{}:{}:{}", self.message_id, self.from, self.to, self.timestamp, msg_typepub fn verify_authentication(&self, auth: &NodeAuth) -> bool { if let Some(ref expected_hmac) = self.hmac { - // Recreate the same data format used for HMAC generation - let msg_type = match &self.message { - RaftMessage::AppendEntries(_) => "AppendEntries".to_string(), - RaftMessage::AppendEntriesResponse(_) => "AppendEntriesResponse".to_string(), - RaftMessage::Vote(_) => "Vote".to_string(), - RaftMessage::VoteResponse(_) => "VoteResponse".to_string(), - RaftMessage::InstallSnapshot(_) => "InstallSnapshot".to_string(), - RaftMessage::InstallSnapshotResponse(_) => "InstallSnapshotResponse".to_string(), - RaftMessage::Heartbeat { from, term } => format!("Heartbeat:{}:{}", from, term), - RaftMessage::HeartbeatResponse { from, success } => { - format!("HeartbeatResponse:{}:{}", from, success) - } - }; + let msg_type = self.message_type_for_hmac(); let data_for_hmac = format!( "{}:{}:{}:{}:{}", self.message_id, self.from, self.to, self.timestamp, msg_typeAlso applies to: 411-422
src/storage/src/lib.rs (1)
42-43: Public API expansion: consider feature-gating or sealingBinlogBatchuntil it’s real.Re-exporting
BinlogBatchmakes it part of the stable surface; if it’s intentionally a placeholder, consider hiding behind aclusterfeature or documenting “unstable/experimental” to avoid long-term API constraints.Also applies to: 69-70
src/storage/src/redis_hashes.rs (1)
293-314: Consider de-duplicating the repeatedcreate_new_hashclosures.The same “init meta + write 1+ fields via batch” logic is repeated in
hset,hmset,hsetnx,hincrby,hincrbyfloat. A small private helper (or a generic helper taking an iterator of(field,value_bytes)) would cut repetition and reduce risk of divergence.Also applies to: 637-665, 779-800, 904-927, 1060-1082
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/raft/src/network.rssrc/storage/src/batch.rssrc/storage/src/lib.rssrc/storage/src/redis.rssrc/storage/src/redis_hashes.rssrc/storage/src/redis_lists.rssrc/storage/src/redis_sets.rssrc/storage/src/redis_strings.rssrc/storage/src/redis_zsets.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/storage/src/redis.rs
🧰 Additional context used
🧬 Code graph analysis (4)
src/storage/src/redis_lists.rs (3)
src/engine/src/rocksdb_engine.rs (1)
db(38-40)src/storage/src/storage_impl.rs (1)
key(150-150)src/storage/src/base_key_format.rs (1)
key(88-90)
src/storage/src/redis_zsets.rs (1)
src/storage/src/batch.rs (2)
new(113-125)new(221-223)
src/storage/src/batch.rs (2)
src/storage/src/redis.rs (1)
new(115-149)src/engine/src/rocksdb_engine.rs (1)
db(38-40)
src/storage/src/redis_strings.rs (1)
src/engine/src/rocksdb_engine.rs (1)
db(38-40)
🪛 GitHub Check: cargo clippy (ubuntu-latest)
src/storage/src/redis_hashes.rs
[failure] 119-119:
this expression creates a reference which is immediately dereferenced by the compiler
[failure] 350-350:
this expression creates a reference which is immediately dereferenced by the compiler
[failure] 400-400:
this expression creates a reference which is immediately dereferenced by the compiler
[failure] 698-698:
this expression creates a reference which is immediately dereferenced by the compiler
[failure] 751-751:
this expression creates a reference which is immediately dereferenced by the compiler
[failure] 836-836:
this expression creates a reference which is immediately dereferenced by the compiler
[failure] 869-869:
this expression creates a reference which is immediately dereferenced by the compiler
[failure] 963-963:
this expression creates a reference which is immediately dereferenced by the compiler
[failure] 1025-1025:
this expression creates a reference which is immediately dereferenced by the compiler
[failure] 1119-1119:
this expression creates a reference which is immediately dereferenced by the compiler
🪛 GitHub Check: lint
src/storage/src/redis_hashes.rs
[failure] 119-119:
this expression creates a reference which is immediately dereferenced by the compiler
[failure] 350-350:
this expression creates a reference which is immediately dereferenced by the compiler
[failure] 400-400:
this expression creates a reference which is immediately dereferenced by the compiler
[failure] 698-698:
this expression creates a reference which is immediately dereferenced by the compiler
[failure] 751-751:
this expression creates a reference which is immediately dereferenced by the compiler
[failure] 836-836:
this expression creates a reference which is immediately dereferenced by the compiler
[failure] 869-869:
this expression creates a reference which is immediately dereferenced by the compiler
[failure] 963-963:
this expression creates a reference which is immediately dereferenced by the compiler
[failure] 1025-1025:
this expression creates a reference which is immediately dereferenced by the compiler
[failure] 1119-1119:
this expression creates a reference which is immediately dereferenced by the compiler
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test-macos
- GitHub Check: test
- GitHub Check: test-windows
- GitHub Check: build and test (macos-latest)
- GitHub Check: build and test (windows-latest)
🔇 Additional comments (8)
src/storage/src/redis_zsets.rs (4)
29-31: Import change looks fine.
321-327:zincrbybatch migration looks correct (delete old score key + write new pair + commit).Also applies to: 383-410
792-877:zrembatching/commit-on-change looks good.
1473-1574: Range-removal paths migrated cleanly to batch+commit.Also applies to: 1670-1697, 1712-1800
src/storage/src/redis_sets.rs (3)
25-26: Import change looks fine.
151-169:saddbatch refactor looks correct (single commit for meta + members).
635-659:spop/smovebatching looks good; multi-key lock ordering insmoveis a nice touch.Also applies to: 795-837
src/storage/src/redis_strings.rs (1)
318-325: Single-key write paths migrated cleanly to batch+commit.Also applies to: 389-396, 581-588, 635-642, 698-705, 759-766, 831-838, 1212-1219, 1284-1291, 1836-1884, 1938-1945, 1979-1986
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/raft/src/network.rs (1)
383-405: Critical: HMAC payload doesn’t cover message body (integrity bypass).Right now
msg_typeis the only commitment toself.messagefor most variants, so an attacker can alter (e.g.)AppendEntriesfields without changingdata_for_hmac, and the HMAC would still verify. Please include the fullRaftMessage(or a digest of it) in the authenticated bytes, and centralize the construction to avoid drift.Proposed fix (commit HMAC to full message content; remove duplicated match)
impl MessageEnvelope { + fn hmac_payload(&self) -> RaftResult<Vec<u8>> { + // IMPORTANT: commit to the full message content (not just its variant name) + bincode::serialize(&(self.message_id, self.from, self.to, self.timestamp, &self.message)) + .map_err(|e| { + RaftError::Serialization(serde_json::Error::io(std::io::Error::new( + std::io::ErrorKind::InvalidData, + e.to_string(), + ))) + }) + } + /// Add authentication to the message pub fn add_authentication(&mut self, auth: &NodeAuth) -> RaftResult<()> { - // Create serializable data without HMAC for authentication - // Use a simplified message representation for HMAC - let msg_type = match &self.message { - RaftMessage::AppendEntries(_) => "AppendEntries".to_string(), - RaftMessage::AppendEntriesResponse(_) => "AppendEntriesResponse".to_string(), - RaftMessage::Vote(_) => "Vote".to_string(), - RaftMessage::VoteResponse(_) => "VoteResponse".to_string(), - RaftMessage::InstallSnapshot(_) => "InstallSnapshot".to_string(), - RaftMessage::InstallSnapshotResponse(_) => "InstallSnapshotResponse".to_string(), - RaftMessage::Heartbeat { from, term } => format!("Heartbeat:{}:{}", from, term), - RaftMessage::HeartbeatResponse { from, success } => { - format!("HeartbeatResponse:{}:{}", from, success) - } - }; - let data_for_hmac = format!( - "{}:{}:{}:{}:{}", - self.message_id, self.from, self.to, self.timestamp, msg_type - ); - - self.hmac = Some(auth.generate_hmac(data_for_hmac.as_bytes())?); + let payload = self.hmac_payload()?; + self.hmac = Some(auth.generate_hmac(&payload)?); Ok(()) } /// Verify message authentication pub fn verify_authentication(&self, auth: &NodeAuth) -> bool { if let Some(ref expected_hmac) = self.hmac { - // Recreate the same data format used for HMAC generation - let msg_type = match &self.message { - RaftMessage::AppendEntries(_) => "AppendEntries".to_string(), - RaftMessage::AppendEntriesResponse(_) => "AppendEntriesResponse".to_string(), - RaftMessage::Vote(_) => "Vote".to_string(), - RaftMessage::VoteResponse(_) => "VoteResponse".to_string(), - RaftMessage::InstallSnapshot(_) => "InstallSnapshot".to_string(), - RaftMessage::InstallSnapshotResponse(_) => "InstallSnapshotResponse".to_string(), - RaftMessage::Heartbeat { from, term } => format!("Heartbeat:{}:{}", from, term), - RaftMessage::HeartbeatResponse { from, success } => { - format!("HeartbeatResponse:{}:{}", from, success) - } - }; - let data_for_hmac = format!( - "{}:{}:{}:{}:{}", - self.message_id, self.from, self.to, self.timestamp, msg_type - ); - - auth.verify_hmac(data_for_hmac.as_bytes(), expected_hmac) + let payload = match self.hmac_payload() { + Ok(v) => v, + Err(_) => return false, + }; + auth.verify_hmac(&payload, expected_hmac) } else { false } } }Also applies to: 408-432
src/storage/src/redis_sets.rs (1)
2375-2398: Potential atomicity issue: two separate batch commits.Similar to
sdiffstore, this helper method performs two sequential batch commits: clearing the destination (lines 2375-2383 or 2387-2394) and adding members viasadd(line 2398). This breaks atomicity forsinterstoreandsunionstoreoperations that depend on this helper.Both clearing and adding operations should be combined into a single atomic batch to prevent partial state if the second operation fails.
src/storage/src/redis_zsets.rs (3)
59-65: Bug: score equality check inzaddcompares the wrong value.
if (existing_score, sm.score).1.abs() < f64::EPSILONevaluatessm.score.abs(), not(|existing_score - sm.score|). This makes “no-op when score unchanged” behave incorrectly and triggers unnecessary delete+put.Proposed fix
- if (existing_score, sm.score).1.abs() < f64::EPSILON { + if (existing_score - sm.score).abs() < f64::EPSILON { // Score is the same, skip continue; } else {Also applies to: 101-180, 188-213
321-327:zincrby: batch update is good, but lock-dropping + re-callingzaddcan break atomicity under concurrency.When meta is invalid/missing, the function drops
_lockthen callszadd, allowing another writer to interleave. Consider an internalzadd_no_lock(or a shared helper that accepts “lock already held”) to keep the whole operation linearizable.Also applies to: 383-410
1234-1265:zinterstore/zunionstore: destination delete + laterzaddis not atomic.You delete destination under a lock, release the lock, then later
zaddrepopulates. Another operation ondestinationcan slip in between. If strict Redis semantics matter here, this should be refactored to a single locked write plan.src/storage/src/redis_lists.rs (1)
319-334: Metadata decrement should match actual keys found; add validation or assertions when data keys are missing.
keys_to_deleteis populated only whenget_cfreturns a value, butpop_countunconditionally decrements the metadata count and indices. If any expected data key is absent, the count will drift even though fewer items were actually popped. The same issue exists in RPOP, RPOPLPUSH, and LREM. Either assert that all keys exist (if guaranteed by design) or adjust the count decrement to matchkeys_to_delete.len()orresult.len().Also applies to: 341-349, 390-405, 412-420, 461-476, 483-491
🧹 Nitpick comments (7)
src/storage/src/redis_strings.rs (3)
581-588: Batch migration for single-key writes looks consistent (put/delete + commit).Nice mechanical refactor: each mutation now commits through the Batch abstraction, aligning with the PR goal (standalone vs cluster).
Consider a tiny helper to reduce repetition/error-surface (create batch → put MetaCF → commit), since this pattern appears many times in this file.
Also applies to: 635-642, 698-705, 759-766, 831-838, 1212-1219, 1284-1291, 1429-1436, 1836-1843, 1856-1863, 1877-1884, 1938-1945, 1979-1986
2110-2151:del_key: good “collect then batch delete”, but watch memory/latency for large composite keys.Collecting all derived keys across CFs into
keys_to_deletecan be large and then written as one huge batch; consider chunked commits (e.g., commit every N deletions) to cap memory and avoid oversized WriteBatch.
2226-2261:flush_db: same batching concern—consider chunking or CF-level primitives.For large DBs, collecting all keys into memory is costly. If RocksDB drop/truncate CF isn’t an option here, chunked batch commits would at least bound memory.
src/storage/src/redis_lists.rs (2)
718-719:ltrim: meta delete vs update is clear; consider chunking deletes for big lists.Logic is readable and the batch write is clean. For very large lists, the delete-key accumulation can be big; chunking commits would reduce peak memory.
Also applies to: 754-757, 764-767, 773-774, 781-782, 791-792, 794-806
888-892:lrem: batch rewrite is fine; watch O(N) memory for large lists.Reads all elements and rewrites the whole list; if that’s already the intended approach, the batch refactor is consistent. If not, this is a hotspot to optimize later.
Also applies to: 898-904, 914-915, 923-937
src/storage/src/redis_hashes.rs (2)
293-314:hset: batch writes look good; consider deduplicating the “create_new_hash” logic across methods.The inline closure pattern repeats in
hset,hmset,hsetnx,hincrby,hincrbyfloat. A small private helper (e.g.,fn init_hash_with_fields(&self, ...)) would reduce drift risk.Also applies to: 346-358, 376-383, 395-408, 414-416
904-926: Error messages for numeric overflow/underflow should be consistent across string and hash operations.There are inconsistencies in error messaging:
- Integer overflow: hashes report
"integer overflow"(line 994) while strings report"increment or decrement would overflow"(strings.rs:1203)- Non-integer parse errors: hashes report
"hash value is not an integer"(line 987) while strings report"value is not an integer or out of range"(strings.rs:1191)- Float parse errors and NaN/Infinity checks are already aligned
For consistency with Redis semantics and client expectations, align these messages across both data types.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/raft/src/network.rssrc/storage/src/batch.rssrc/storage/src/lib.rssrc/storage/src/redis.rssrc/storage/src/redis_hashes.rssrc/storage/src/redis_lists.rssrc/storage/src/redis_sets.rssrc/storage/src/redis_strings.rssrc/storage/src/redis_zsets.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/storage/src/lib.rs
- src/storage/src/redis.rs
- src/storage/src/batch.rs
🧰 Additional context used
🧬 Code graph analysis (3)
src/storage/src/redis_strings.rs (2)
src/storage/src/redis.rs (1)
new(115-149)src/engine/src/rocksdb_engine.rs (1)
db(38-40)
src/storage/src/redis_hashes.rs (3)
src/storage/src/base_meta_value_format.rs (2)
new(56-63)new(103-147)src/storage/src/base_key_format.rs (1)
key(88-90)src/storage/src/redis_multi.rs (1)
exists(130-152)
src/storage/src/redis_zsets.rs (1)
src/storage/src/batch.rs (2)
new(113-125)new(221-223)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: cargo clippy (macos-latest)
- GitHub Check: build and test (macos-latest)
- GitHub Check: build and test (windows-latest)
🔇 Additional comments (15)
src/storage/src/redis_sets.rs (5)
25-25: LGTM! Clean import refactoring.The removal of
WriteBatchfrom the imports is correct and aligns with the batch abstraction introduced in this PR.
151-168: LGTM! Atomic batch operations correctly implemented.The refactoring properly uses the batch abstraction with a single atomic commit for all metadata and member updates.
472-523: LGTM! Proper batch accumulation pattern.The code correctly collects keys to delete, then applies all deletions and metadata updates in a single atomic batch commit.
635-659: LGTM! Atomic batch operations for member removal.The batch correctly accumulates all member deletions and metadata updates before committing atomically.
795-837: LGTM! Atomic cross-set move operation.The batch correctly ensures atomicity for the move operation across both source and destination sets with a single commit.
src/storage/src/redis_lists.rs (2)
105-108:push_core: delete/put ordering and meta update in the same commit looks right.Collecting
deletesthenputs, and finally writing meta in the same batch should preserve the intended atomicity for list mutations (especially when reorganizing and bumping version).Also applies to: 125-126, 156-157, 172-173, 184-185, 208-209, 224-225, 236-246
1032-1035:linsert: version bump + delete old version keys + write new version keys is a solid pattern.This avoids in-place shifting complexity and keeps atomicity via a single batch commit.
Also applies to: 1041-1042, 1056-1057, 1063-1073
src/storage/src/redis_zsets.rs (3)
30-30: Import change only.
792-798:zrem: batch delete + meta update in one commit is correct.Nice that meta removal when count hits zero is handled in the same batch.
Also applies to: 824-877
1473-1479: Range removals: batched member+score deletions + meta update are consistent.Pattern is uniform and easy to reason about after the refactor.
Also applies to: 1547-1570, 1670-1693, 1712-1718, 1773-1796
src/storage/src/redis_hashes.rs (4)
23-23: Import change only.
86-122:hdel: collect-then-batch delete is solid.Good: existence checks are done on the snapshot, then deletes + meta update are committed together via Batch.
637-665:hmset: good batching and correct “update meta only if new fields added” optimization.
data_to_write+ conditional meta put keeps writes minimal while still committing atomically.Also applies to: 694-711, 715-755, 758-759
779-800:hsetnx: batch path matches expected semantics.The new-field path updates meta + data in one commit; existing field returns early without writes.
Also applies to: 832-844, 865-877, 883-885
src/storage/src/redis_strings.rs (1)
318-325: No issue detected. MetaCF is explicitly the default RocksDB column family (mapped to "default" in the CF name function). Reads viadb.get_opt()and writes viabatch.put(ColumnFamilyIndex::MetaCF, ...)both target the same CF, so there is no read/write inconsistency.
Tsukikage7
force-pushed
the
feature/support-batch
branch
from
0fd2c70 to
a5b2f2f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/storage/src/redis_lists.rs (1)
1080-1154:RPOPLPUSHis not atomic at the storage level (two separate batch commits).Even though locks prevent concurrent interleavings, a crash between
rpop_internal(...).commit()andpush_core(...).commit()can lose the element or leave source/destination inconsistent. This is a core Redis guarantee forRPOPLPUSH/BRPOPLPUSH.Consider a dedicated internal implementation that:
- reads source tail element,
- updates/deletes source element + source meta,
- writes destination head element + destination meta,
- and commits once with a single batch spanning both keys.
src/storage/src/redis_zsets.rs (2)
99-180: Fix incorrect float “same score” check inzadd(it’s currently wrong).
(existing_score, sm.score).1.abs()is justabs(sm.score), not the delta. This can skip updates incorrectly (especially for scores near 0).Proposed fix
- if (existing_score, sm.score).1.abs() < f64::EPSILON { + if (existing_score - sm.score).abs() < f64::EPSILON { // Score is the same, skip continue; } else {
1094-1278: ZINTERSTORE/ZUNIONSTORE destination update is not atomic (visible intermediate state).
zset_store_operationcommits deletion of the destination, releases the lock, and later callszadd(which commits separately). Other operations can observe “destination missing/empty” between these commits, which violates Redis atomicity expectations for these commands.Suggestion: keep the destination lock through the full operation and write the final destination state in one commit (either by an internal no-lock
zaddvariant, or by assembling the final batch directly).src/storage/src/redis_strings.rs (1)
1063-1155: Add multi-key locking to msetnx to prevent TOCTOU race condition.Without acquiring locks on all keys before the existence check, another thread can insert a key between msetnx's check phase and write phase, violating the atomic "set none if any exists" semantics. Acquire and hold locks on all keys (sorted to avoid deadlocks) throughout the entire operation, matching the pattern already used for multi-key operations elsewhere in the codebase.
Proposed direction (sketch)
pub fn msetnx(&self, kvs: &[(Vec<u8>, Vec<u8>)]) -> Result<bool> { + // Acquire locks for all keys (sorted) to prevent TOCTOU under concurrency. + let mut key_strs: Vec<String> = kvs + .iter() + .map(|(k, _)| String::from_utf8_lossy(k).to_string()) + .collect(); + key_strs.sort(); + key_strs.dedup(); + let _locks: Vec<ScopeRecordLock> = key_strs + .iter() + .map(|ks| ScopeRecordLock::new(self.lock_mgr.as_ref(), ks)) + .collect(); + let db = self.db.as_ref().context(OptionNoneSnafu { message: "db is not initialized".to_string(), })?;
🤖 Fix all issues with AI agents
In @src/storage/src/redis_sets.rs:
- Around line 2315-2330: In clear_and_store_set, initial_meta_value() resets the
count to zero which causes stored metadata to have count=0; after creating
new_set_meta (ParsedSetsMetaValue) and before encoding/writing it (before
batch.put), call new_set_meta.modify_count(member_count) (or otherwise set the
count via ParsedSetsMetaValue::modify_count) so the correct member_count is
preserved instead of being overwritten by initial_meta_value().
🧹 Nitpick comments (5)
src/storage/src/redis_strings.rs (2)
318-325: Reduce repeated “single-put then commit” boilerplate (and keep semantics consistent).This pattern is now repeated across many string write paths; consider a small helper on
Redislikeput_meta_and_commit(key, value)(orwith_batch(|b| ...)) to centralize error context and avoid drift (e.g., future write options / metrics / batch-size guards).Also applies to: 389-396, 581-588, 635-642, 698-705, 759-766, 831-838, 1212-1219, 1284-1291, 1429-1436, 1836-1843, 1856-1863, 1877-1884, 1938-1945, 1979-1986
2110-2151: Unbounded key collection + huge single batch indel_key/flush_dbcan be risky.Both functions can accumulate very large
keys_to_deleteand then issue a singlebatch.commit(). This can spike memory and potentially exceed RocksDBWriteBatchpractical limits. Consider:
- streaming deletes directly into the batch while iterating (if borrow rules allow), and/or
- chunking commits for
flush_db(atomicity isn’t usually required there), and/or- a RocksDB-native approach (e.g., drop+recreate CF,
delete_range, etc.) depending on your Engine API.Also applies to: 2226-2261
src/storage/src/redis_hashes.rs (2)
86-122: Avoid extra allocations when collecting encoded keys for deletion.
encoded_keyis already an ownedVec<u8>;keys_to_delete.push(encoded_key.to_vec())clones again. Prefer pushingencoded_keydirectly (or store asVec<Vec<u8>>and move it).
293-314: De-duplicatecreate_new_hashclosures into a single helper.The repeated closure blocks are easy to desync over time (meta encoding/versioning, batch wiring, etc.). A private
fn create_hash_with_fields(&self, key, fields_iter) -> Result<()>would reduce risk and simplify future cluster-mode adjustments.Also applies to: 637-665, 779-800, 904-926, 1060-1082
src/storage/src/redis_sets.rs (1)
2258-2287: Consider adding a comment about intentional type overwrite behavior.The code at line 2259 checks if the existing destination is a Set type, but doesn't error if it's a different type—it simply skips collecting old members. This means a non-Set destination gets overwritten with a Set, which matches Redis semantics for
SDIFFSTORE/SINTERSTORE/SUNIONSTORE.A brief comment clarifying this intentional behavior would improve maintainability.
📝 Suggested documentation improvement
// Delete existing destination set metadata and data let meta_val = db .get_cf(&cf_meta, &dest_base_meta_key) .context(RocksSnafu)?; if let Some(val) = meta_val { + // Only collect old members if destination is a valid Set. + // Per Redis semantics, *STORE commands overwrite any existing key type. if val.first().copied() == Some(DataType::Set as u8) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/raft/src/network.rssrc/storage/src/batch.rssrc/storage/src/lib.rssrc/storage/src/redis.rssrc/storage/src/redis_hashes.rssrc/storage/src/redis_lists.rssrc/storage/src/redis_sets.rssrc/storage/src/redis_strings.rssrc/storage/src/redis_zsets.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/storage/src/lib.rs
- src/raft/src/network.rs
- src/storage/src/redis.rs
- src/storage/src/batch.rs
🧰 Additional context used
🧬 Code graph analysis (2)
src/storage/src/redis_zsets.rs (1)
src/storage/src/batch.rs (2)
new(113-125)new(221-223)
src/storage/src/redis_sets.rs (3)
src/storage/src/base_meta_value_format.rs (3)
new(56-63)new(103-147)encoded(234-236)src/storage/src/member_data_key_format.rs (4)
new(42-50)new(98-134)key(136-138)version(140-142)src/storage/src/base_key_format.rs (3)
new(42-48)new(69-73)key(88-90)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: test
- GitHub Check: test-windows
- GitHub Check: test-macos
- GitHub Check: build and test (macos-latest)
- GitHub Check: cargo clippy (macos-latest)
- GitHub Check: build and test (ubuntu-latest)
- GitHub Check: build and test (windows-latest)
- GitHub Check: cargo clippy (windows-latest)
🔇 Additional comments (9)
src/storage/src/redis_lists.rs (1)
105-246: Batching approach inpush_corelooks coherent (delete → put → meta → commit).The collected
deletesandputsapplied in a single commit is aligned with the new Batch abstraction and avoids borrow conflicts.src/storage/src/redis_strings.rs (1)
581-588: No action needed. Reads and writes are correctly aligned to the same column family.The
Engine::get_opt()method reads from RocksDB's default column family (when no CF is explicitly specified), and writes viabatch.put(ColumnFamilyIndex::MetaCF, ...)correctly target the same CF sinceMetaCFmaps to RocksDB's"default"column family. The batch implementation properly converts theMetaCFindex to the corresponding CF handle and commits viaput_cf(). There is no read/write inconsistency.src/storage/src/redis_sets.rs (7)
25-25: LGTM!Import correctly updated to remove
WriteBatchsince batch operations now use the abstractedBatchtrait viaself.create_batch().
151-168: LGTM!The batch abstraction is correctly applied: creates batch via factory method, accumulates metadata and member data puts, then commits atomically. Error handling properly propagates through
?.
497-519: LGTM!Good implementation pattern: batch is created only when needed (
removed_count > 0), keys are collected first then batch-deleted, and metadata update/deletion is handled conditionally. All operations are committed atomically.
635-658: LGTM!Batch operations correctly follow the early return check for empty
members_to_pop. Member deletions and metadata updates are accumulated and committed atomically.
795-836: LGTM!Complex cross-set operation (source → destination) is handled correctly with proper lock ordering to prevent deadlocks. All operations—source member deletion, destination member addition, and both metadata updates—are accumulated in a single batch and committed atomically.
2177-2191: LGTM!Clean separation of concerns:
sdiffstorecalculates the difference and delegates the atomic store operation to theclear_and_store_sethelper.
2225-2229: Good documentation of atomicity guarantee.The method documentation clearly explains the atomicity contract, and the implementation correctly uses a single batch with one commit point to fulfill this guarantee.
Tsukikage7
force-pushed
the
feature/support-batch
branch
from
a5b2f2f to
6caf66c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/storage/src/redis_lists.rs (2)
1095-1169:rpoplpushis no longer crash-atomic (two separate commits).Even with locks preventing interleaving, a crash after
rpop_internalcommits but beforepush_corecommits can lose the element (and violates Redis RPOPLPUSH atomicity expectations). If possible, build a single batch that updates both source and destination list data + metas and commit once.
105-246: Document or guard theleft_indexarithmetic to prevent u64 underflow.The code performs unchecked subtraction on
left_index(e.g., line 118:left_index - values.len()andmodify_left_indexin list_meta_value_format.rs line 289). While the starting value ofINITIAL_LEFT_INDEX = i64::MAXprovides ample room in practice, add either: (1) a code comment documenting the invariant thatleft_indexwill not decrease by more than i64::MAX elements over the list's lifetime, or (2) usechecked_subwith a version bump when approaching a rebase threshold.src/storage/src/redis_zsets.rs (2)
117-123: Critical bug: score “equality” check is wrong ((existing_score, sm.score).1.abs()).This currently checks
abs(sm.score) < EPSILON, notabs(existing_score - sm.score) < EPSILON, so it will skip updates only when the new score is near 0.0.Proposed fix
- if (existing_score, sm.score).1.abs() < f64::EPSILON { + if (existing_score - sm.score).abs() < f64::EPSILON { // Score is the same, skip continue; } else {
1215-1278:ZINTERSTORE/ZUNIONSTOREdelete-then-zadd sequence is not crash-atomic.
zset_store_operationcommits deletion of the destination, releases the lock, then callszadd(another commit). A crash between them leaves destination empty. If Redis-like atomicity is desired, consider writing the computed destination content in a single batch (or using a “write new version then swap meta pointer” approach).
🤖 Fix all issues with AI agents
In @src/storage/src/redis_sets.rs:
- Around line 2305-2327: The code uses
ParsedSetsMetaValue::modify_count(member_count) after calling
new_set_meta.initial_meta_value(), but modify_count() applies a delta
(saturating_add) rather than setting the count directly; replace the call to
modify_count(member_count) with set_count(member_count) so the metadata count is
explicitly set after initialization (refer to ParsedSetsMetaValue,
initial_meta_value(), modify_count(), and set_count()).
🧹 Nitpick comments (6)
src/storage/src/batch.rs (1)
201-264: BinlogBatch:put/deletereturningOk(())is easy to misuse in cluster mode.Since cluster mode is “not implemented”, consider failing fast in
put/deletetoo (not justcommit) to avoid callers accumulating lots of in-memory “successful” operations before discovering the error at the end. If you intentionally want to allow building the log first, please add a loud doc comment onBinlogBatchand/or onRedis::create_batch()cluster branch.src/storage/src/redis_strings.rs (3)
318-325: Good migration tocreate_batch(); consider a small helper to remove repetitiveput+commitboilerplate.There are many single-key write paths now doing
create_batch() -> put(MetaCF, ...) -> commit(). A helper likeself.meta_put(key, value)(orself.commit_single_put(cf, k, v)) would reduce duplication and keep error contexts consistent.Also applies to: 389-396, 581-588, 635-642, 698-705, 759-766, 831-838, 1063-1079, 1138-1155, 1212-1219, 1284-1291, 1429-1436, 1836-1843, 1856-1863, 1877-1884, 1938-1945, 1979-1986
2110-2151:del_key/flush_db: collecting all keys before deleting can OOM or create oversized batches.For large datasets,
keys_to_deletecan grow without bound and the resulting RocksDB WriteBatch can become huge. Consider chunking (e.g., commit every N deletions) while iterating.Also applies to: 2226-2261
1106-1111: Redundant MetaCF handle fetch inmsetnx.
let _cf = ...get_cf_handle(MetaCF)...?;is only used as an initialization check;batch.put(MetaCF, ...)should already fail if MetaCF is missing. If you keep it, consider a brief comment clarifying it’s intentional.src/storage/src/redis_hashes.rs (1)
293-314: Code duplication:create_new_hashhelper closures are duplicated across multiple functions.The
create_new_hashclosure is defined nearly identically inhset,hmset,hsetnx,hincrby, andhincrbyfloat. Consider extracting this to a shared private method to reduce code duplication and improve maintainability.♻️ Suggested refactor
Extract the common logic into a private helper method:
/// Helper to create a new hash with a single field fn create_new_hash_single_field( &self, key: &[u8], base_meta_key: &[u8], field: &[u8], value: &[u8], ) -> Result<()> { let mut hashes_meta = HashesMetaValue::new(Bytes::copy_from_slice(&1u64.to_le_bytes())); hashes_meta.inner.data_type = DataType::Hash; let version = hashes_meta.update_version(); let data_key = MemberDataKey::new(key, version, field); let data_value = BaseDataValue::new(value.to_vec()); let mut batch = self.create_batch()?; batch.put(ColumnFamilyIndex::MetaCF, base_meta_key, &hashes_meta.encode())?; batch.put(ColumnFamilyIndex::HashesDataCF, &data_key.encode()?, &data_value.encode())?; batch.commit() }src/storage/src/redis_sets.rs (1)
2062-2066: Minor: Unused variable_cf_data.The variable
_cf_datais retrieved but never used insinter. Sincesismemberis called which retrieves its own handles, this lookup is unnecessary.♻️ Suggested fix
- let _cf_data = - self.get_cf_handle(ColumnFamilyIndex::SetsDataCF) - .context(OptionNoneSnafu { - message: "cf data is not initialized".to_string(), - })?;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/raft/src/network.rssrc/storage/src/batch.rssrc/storage/src/lib.rssrc/storage/src/redis.rssrc/storage/src/redis_hashes.rssrc/storage/src/redis_lists.rssrc/storage/src/redis_sets.rssrc/storage/src/redis_strings.rssrc/storage/src/redis_zsets.rssrc/storage/tests/redis_list_test.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/storage/src/lib.rs
- src/storage/src/redis.rs
- src/raft/src/network.rs
🧰 Additional context used
🧬 Code graph analysis (4)
src/storage/src/redis_strings.rs (2)
src/storage/src/redis.rs (1)
new(115-149)src/engine/src/rocksdb_engine.rs (1)
db(38-40)
src/storage/tests/redis_list_test.rs (1)
src/storage/src/raft_integration.rs (1)
create_test_redis(100-112)
src/storage/src/batch.rs (1)
src/engine/src/rocksdb_engine.rs (1)
db(38-40)
src/storage/src/redis_sets.rs (4)
src/storage/src/base_meta_value_format.rs (3)
new(56-63)new(103-147)encoded(234-236)src/storage/src/member_data_key_format.rs (4)
new(42-50)new(98-134)key(136-138)version(140-142)src/storage/src/base_key_format.rs (3)
new(42-48)new(69-73)key(88-90)src/storage/src/base_data_value_format.rs (2)
new(40-47)new(75-121)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: test-windows
- GitHub Check: test
- GitHub Check: test-macos
- GitHub Check: cargo clippy (macos-latest)
- GitHub Check: cargo clippy (windows-latest)
- GitHub Check: build and test (macos-latest)
- GitHub Check: build and test (windows-latest)
- GitHub Check: build and test (ubuntu-latest)
🔇 Additional comments (22)
src/storage/src/batch.rs (2)
266-282: Tests cover the “commit must error” guardrail—good.This prevents silent data loss if BinlogBatch is accidentally used.
56-90:Batch: Sendis appropriate;Syncis not needed.Batches are always created as local mutable variables within a single function scope and moved into
commit(self: Box<Self>), consuming them. They are never shared across threads, stored inArc, or accessed from multiple thread contexts. The currentSendbound is sufficient.Likely an incorrect or invalid review comment.
src/storage/tests/redis_list_test.rs (1)
598-617: LPUSHX/RPUSHX expectations now match Redis semantics (non-existent key => 0, no creation).These test updates look consistent with Redis behavior and align with the new “delete meta when empty” list implementation.
Also applies to: 625-645
src/storage/src/redis_lists.rs (2)
317-354: Deleting list metadata when empty is the right Redis-compatible behavior.This directly supports the updated LPUSHX/RPUSHX semantics and makes
EXISTSbehave as expected for empty lists.Also applies to: 393-430, 469-506
733-823: Batching across complex list mutations looks consistent (delete first, then put, then meta).The pattern of collecting keys, applying deletions before writes, and committing once is a good fit for RocksDB atomic batches.
Also applies to: 902-952, 1047-1088
src/storage/src/redis_zsets.rs (1)
101-180: Batch migration for ZSET writes is clean and consistent.Using a single batch for “delete old score key + write new member/score + meta update” is the right approach for consistency between
ZsetsDataCFandZsetsScoreCF.Also applies to: 188-214, 383-410, 824-877, 1547-1575, 1670-1698, 1773-1801
src/storage/src/redis_hashes.rs (9)
23-23: Import cleanup looks correct.The
WriteBatchimport was removed since direct WriteBatch usage is replaced with the batch abstraction.
86-121: Batch abstraction correctly applied for hdel.The refactoring properly:
- Collects keys to delete first
- Creates a single batch for all operations
- Commits atomically with both data deletions and metadata update
This ensures atomicity for the delete operation.
346-357: Batch operations correctly implemented for stale/empty hash case.The batch correctly groups the metadata update and data write into a single atomic commit.
376-382: Batch correctly used for value update case.Single field update is properly batched and committed.
396-407: Batch correctly used for new field insertion.Both metadata count update and data insertion are properly grouped in a single batch commit.
648-664: hmset helper correctly batches multiple field writes.The helper properly creates a batch, writes metadata, then iterates through all field-value pairs adding them to the batch before a single commit.
715-754: Efficient batching pattern for hmset with existing hash.The implementation correctly:
- Collects all data to write first (line 716)
- Checks field existence to track new fields count
- Creates a single batch for all writes
- Only updates metadata if new fields were added (line 747-752)
This ensures minimal overhead and atomic writes.
1000-1006: Batch correctly applied for hincrby existing value update.Single atomic commit for the updated value.
1159-1165: Batch correctly applied for hincrbyfloat existing value update.Consistent with the hincrby pattern.
src/storage/src/redis_sets.rs (7)
25-25: Import correctly updated.
WriteBatchremoved from imports since the batch abstraction is now used instead.
151-168: Batch abstraction correctly applied for sadd.The implementation properly batches metadata and all member additions before a single commit, ensuring atomicity.
472-519: Batch abstraction correctly applied for srem.The refactoring properly:
- Collects keys to delete (line 472)
- Creates batch and deletes all collected keys
- Conditionally deletes or updates metadata
- Commits atomically
This ensures the remove operation is atomic.
635-658: Batch abstraction correctly applied for spop.All member deletions and metadata updates are properly grouped in a single batch commit.
795-836: Batch abstraction correctly applied for smove.The implementation properly handles:
- Deleting from source set
- Updating source metadata (delete if empty, update count otherwise)
- Adding to destination set (if not already present)
- Updating destination metadata
All operations are in a single atomic batch commit.
2189-2190: Good refactoring to use shared helper.Using
clear_and_store_seteliminates code duplication acrosssdiffstore,sinterstore, andsunionstore.
2225-2349: Well-designed atomic helper method for set store operations.The
clear_and_store_setmethod correctly:
- Locks the destination key
- Collects existing members to delete
- Creates a single batch for all operations
- Deletes old data keys
- Handles empty result case (deletes metadata, commits, returns)
- Deduplicates new members
- Creates new metadata with correct count
- Writes all new members
- Commits atomically
This ensures no intermediate states are visible.
arana-db#222) This commit introduces a Batch trait to abstract batch write operations, enabling support for both standalone (RocksDB) and future cluster (Raft) modes. Changes: - Add batch.rs module with Batch trait, RocksBatch, and BinlogBatch implementations - Add create_batch() method to Redis struct for creating batch instances - Refactor redis_strings.rs to use Batch trait (2 places) - Refactor redis_hashes.rs to use Batch trait (6 places) - Refactor redis_lists.rs to use Batch trait (7 places) - Refactor redis_sets.rs to use Batch trait (6 places) - Refactor redis_zsets.rs to use Batch trait (8 places) - Add explicit error handling for invalid column family index This abstraction allows seamless switching between direct RocksDB writes and Raft consensus-based writes in the future.
Tsukikage7
force-pushed
the
feature/support-batch
branch
from
6caf66c to
7560057
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/storage/src/redis_zsets.rs (2)
99-180: Fix score equality check inzadd—current code compares the wrong value.
This can incorrectly treat scores as “equal” wheneversm.scoreis near 0, even ifexisting_scorediffers.Proposed fix
- if (existing_score, sm.score).1.abs() < f64::EPSILON { + if (existing_score - sm.score).abs() < f64::EPSILON { // Score is the same, skip continue; } else {
1234-1265: Clear and re-add are two separate lock acquisitions with an interleaving window.The code clears the destination (batch deletes + commit) while holding a lock, then releases the lock before calling
zadd(). This creates a brief window where other operations can access an incomplete state ofdestination. If this operation requires atomicity (e.g., atomically replacing all members), consider extending the single lock scope to encompass both the clear and the subsequentzadd()call.src/storage/src/redis_strings.rs (2)
2110-2151:del_keyprefix-scan likely won’t delete composite-type data keys (prefix mismatch).
You useBaseKey::encode()(includes suffix reserve) as the prefix to scan data CFs, but formats likeMemberDataKeydon’t start with the full BaseKey encoding. This meanskeys_to_deletewill usually stay empty for Sets/Hashes/Lists/ZSets data CFs, leaving the comment “works for all data types” inaccurate.Suggested direction
- // Prefix-scan data CF and delete all derived keys - let iter = db.iterator_cf( - &cf, - rocksdb::IteratorMode::From(&encoded, rocksdb::Direction::Forward), - ); + // TODO: Use a key-format-appropriate prefix for each CF (do NOT use BaseKey::encode()). + // For versioned member keys, prefer building prefixes from the meta's version + // (e.g. MemberDataKey::new(key, version, &[]).encode_seek_key()). + let iter = db.iterator_cf( + &cf, + rocksdb::IteratorMode::From(&encoded, rocksdb::Direction::Forward), + );
2226-2261: Consider chunking deletes to avoid O(total_keys) memory accumulation inflush_db().The current implementation collects all keys from all 6 column families into a single Vec before batch deleting, which would load the entire dataset into memory. For large databases, refactor to periodically commit batches (e.g., every N keys) using the existing
batch.clear()method to reset operations between commits.CF drop/recreate or range-delete optimizations are not exposed by the current Engine abstraction, so chunking is the viable approach here.
src/storage/src/redis_lists.rs (1)
720-722: Inconsistent with batch abstraction -lsetbypasses the batch layer.This direct
db.put_cf()call is inconsistent with the batch abstraction pattern used throughout this PR. In cluster mode, this write would bypass the Raft consensus layer since it doesn't go throughBinlogBatch.🐛 Proposed fix
- // Update the element - db.put_cf(lists_data_cf, &encoded_data_key, &encoded_data_value) - .context(RocksSnafu)?; + // Update the element using batch for consistency with cluster mode + let mut batch = self.create_batch()?; + batch.put(ColumnFamilyIndex::ListsDataCF, &encoded_data_key, &encoded_data_value)?; + batch.commit()?;
🤖 Fix all issues with AI agents
In @src/storage/src/redis.rs:
- Around line 320-360: The create_batch() implementation currently builds
cf_handles by calling get_cf_handle(ColumnFamilyIndex::...) which uses enum
indices and can return wrong handles if self.handles was filtered; change it to
resolve column families by name via the DB API instead: call
db.cf_handle(ColumnFamilyIndex::MetaCF.name()) (and the same for HashesDataCF,
SetsDataCF, ListsDataCF, ZsetsDataCF, ZsetsScoreCF), wrap each Option into the
Arc/BoundColumnFamily type expected by RocksBatch, and pass that Vec to
crate::batch::RocksBatch::new; ensure you remove reliance on self.handles or
index-based lookup and keep the same error handling around
self.db.as_ref().context(...).
🧹 Nitpick comments (3)
src/storage/src/redis_hashes.rs (1)
293-314: Consider extracting duplicatecreate_new_hashclosures into a shared helper method.There are 5 nearly identical
create_new_hashclosures acrosshset,hmset,hsetnx,hincrby, andhincrbyfloat. Each follows the same pattern:
- Create
HashesMetaValuewith initial count- Update version
- Create data key/value
- Batch put meta + data
- Commit
This duplication increases maintenance burden. Consider extracting to a private helper method on
Redis.♻️ Suggested approach
impl Redis { /// Helper to create a new hash with a single field fn create_new_hash_single_field( &self, base_meta_key: &[u8], key: &[u8], field: &[u8], value: &[u8], ) -> Result<()> { let mut hashes_meta = HashesMetaValue::new(Bytes::copy_from_slice(&1u64.to_le_bytes())); hashes_meta.inner.data_type = DataType::Hash; let version = hashes_meta.update_version(); let data_key = MemberDataKey::new(key, version, field); let data_value = BaseDataValue::new(value.to_vec()); let mut batch = self.create_batch()?; batch.put(ColumnFamilyIndex::MetaCF, base_meta_key, &hashes_meta.encode())?; batch.put(ColumnFamilyIndex::HashesDataCF, &data_key.encode()?, &data_value.encode())?; batch.commit() } }Also applies to: 637-665, 779-800, 904-926, 1060-1082
src/storage/src/batch.rs (2)
95-98: Consider derivingEXPECTED_CF_COUNTfrom the enum to avoid manual sync.The hardcoded constant relies on developers remembering to update it when adding new column families. While
cf_index_to_usize's exhaustive match provides compile-time safety for the mapping, the count itself could get out of sync.♻️ Alternative approach using a const fn
If
ColumnFamilyIndexderives common traits, you could potentially use:// In the enum definition file impl ColumnFamilyIndex { pub const COUNT: usize = 6; // Or use a macro to count variants }Then reference
ColumnFamilyIndex::COUNTinstead of a separate constant. This keeps the count co-located with the enum definition, making it more likely to be updated together.Also applies to: 130-137
258-280: BinlogBatch silently accepts operations but fails on commit - consider failing early.Currently
putanddeletereturnOk(())butcommitfails. This could lead to confusing debugging scenarios where many operations "succeed" before finally failing at commit.Consider either:
- Return an error immediately on
put/deleteto fail fast- Add a warning log when operations are added to an unimplemented batch
♻️ Option 1: Fail early on put/delete
impl Batch for BinlogBatch { fn put(&mut self, _cf_idx: ColumnFamilyIndex, _key: &[u8], _value: &[u8]) -> Result<()> { - // TODO: Implement when Raft integration is ready - // Create binlog entry and add to entries - self.count += 1; - Ok(()) + BatchSnafu { + message: "BinlogBatch is not implemented - Raft integration pending".to_string(), + } + .fail() }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/raft/src/network.rssrc/storage/src/batch.rssrc/storage/src/lib.rssrc/storage/src/redis.rssrc/storage/src/redis_hashes.rssrc/storage/src/redis_lists.rssrc/storage/src/redis_sets.rssrc/storage/src/redis_strings.rssrc/storage/src/redis_zsets.rssrc/storage/tests/redis_list_test.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/raft/src/network.rs
- src/storage/tests/redis_list_test.rs
🧰 Additional context used
🧬 Code graph analysis (3)
src/storage/src/redis.rs (1)
src/storage/src/batch.rs (1)
get_cf_handle(177-194)
src/storage/src/redis_sets.rs (2)
src/storage/src/member_data_key_format.rs (4)
new(42-50)new(98-134)key(136-138)version(140-142)src/storage/src/base_key_format.rs (3)
new(42-48)new(69-73)key(88-90)
src/storage/src/redis_lists.rs (5)
src/cmd/src/list.rs (14)
new(38-48)new(83-93)new(128-138)new(202-212)new(278-288)new(322-332)new(380-390)new(449-459)new(513-523)new(578-588)new(634-644)new(679-689)new(724-734)new(782-792)src/storage/src/storage.rs (1)
new(79-82)src/engine/src/rocksdb_engine.rs (1)
db(38-40)src/storage/src/storage_impl.rs (1)
key(150-150)src/storage/src/base_key_format.rs (1)
key(88-90)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build and test (ubuntu-latest)
- GitHub Check: cargo clippy (macos-latest)
- GitHub Check: cargo clippy (windows-latest)
- GitHub Check: build and test (windows-latest)
- GitHub Check: build and test (macos-latest)
- GitHub Check: test-windows
- GitHub Check: test-macos
- GitHub Check: test
🔇 Additional comments (18)
src/storage/src/lib.rs (1)
42-43: Public re-export looks consistent with the feature goal; just be aware it’s a semver commitment.
If you want more flexibility later, consider re-exporting onlyBatchand keeping concrete impls behind a feature flag.Also applies to: 69-70
src/storage/src/redis_zsets.rs (5)
30-31: Import change is fine.
188-213: Batch-based create path for new zset looks consistent.
321-410: Batch-based score update inzincrbylooks good (incl. NaN/Inf guard).
792-877:zremnow commits all deletes + meta update atomically—good.
1473-1574: Range-removal paths now batch deletes + meta update—nice consistency.Also applies to: 1670-1697, 1712-1801
src/storage/src/redis_strings.rs (1)
318-325: Batch migration for single-key string mutations is consistent and readable.Also applies to: 389-396, 581-588, 635-642, 698-705, 759-766, 831-838, 1212-1219, 1284-1291, 1429-1436, 1836-1843, 1856-1863, 1877-1884, 1938-1945, 1979-1986
src/storage/src/redis_sets.rs (3)
25-27: Import change is fine.
151-169: Batch usage in core mutation ops (sadd/srem/spop/smove) looks consistent and improves atomicity.Also applies to: 498-520, 635-659, 795-837
2189-2191: The concern about improper destination clearing is not substantiated by the current implementation.The
clear_and_store_set()function properly handles all destination key types:
String destinations: When destination holds a String, the metadata is read from MetaCF. Since the type check at line 2259 identifies it as non-Set, the member deletion block is correctly skipped (Strings have no members in SetsDataCF to delete). The old String metadata is then overwritten atomically at lines 2305-2307 with new Set metadata using the same key encoding.
Set destinations: For existing Sets, old member keys are deleted from SetsDataCF (lines 2264-2290), and the metadata is updated with a new version.
Key storage model: Both String values and Set metadata use the same MetaCF column family with BaseKey/BaseMetaKey encoding. Each key has a single metadata entry; there are no separate "old key(s)" left behind when overwriting a String with a Set—the entry is simply replaced atomically.
The current implementation properly maintains overwrite semantics and prevents stale data through proper version management and atomic batch operations. No type validation or garbage data issues exist in the code.
src/storage/src/redis_hashes.rs (2)
86-121: LGTM - batch operations for hdel are correctly implemented.The refactoring properly collects keys to delete first, then creates the batch and applies all operations atomically. The count update and meta value update are correctly bundled in the same batch.
346-357: LGTM - consistent batch operations across all hash mutation functions.All batch operations correctly:
- Create batch via
self.create_batch()?- Bundle meta and data updates in the same batch for atomicity
- Propagate errors via
?operator- Call
batch.commit()?at the endAlso applies to: 376-407, 693-754, 832-876, 959-1032, 1115-1191
src/storage/src/redis_lists.rs (3)
105-245: LGTM - push_core correctly implements batch operations.The implementation properly:
- Collects puts and deletes in vectors
- Applies deletes before puts (correct order for version transitions)
- Includes metadata update in the same batch
- Handles both empty and non-empty list cases correctly
341-353: LGTM - pop operations correctly implement batch with metadata cleanup.All pop operations correctly:
- Delete data keys in a single batch
- Delete metadata when list becomes empty (Redis-compatible behavior)
- Update metadata otherwise
- Use
batch.commit()for atomic executionAlso applies to: 417-429, 493-505
769-820: LGTM - ltrim, lrem, and linsert correctly implement batch operations.The implementations properly handle:
- Conditional metadata deletion/update via flags
- Collection of puts and deletes before batch creation
- Atomic commit of all operations
Also applies to: 903-951, 1047-1087
src/storage/src/batch.rs (3)
56-90: LGTM - well-designed Batch trait with proper documentation.The trait definition is clean with:
- Clear method signatures for put, delete, commit, count, clear
- Good documentation explaining error handling approach
Sendbound enables cross-thread usagecommit(self: Box<Self>)properly consumes the batch to prevent reuse after commit
196-225: LGTM - RocksBatch implementation is correct and well-structured.The implementation:
- Properly validates CF handles before use via
get_cf_handle- Correctly increments count on put/delete operations
- Uses
context(RocksSnafu)for proper error propagation on commitclear()resets both the inner WriteBatch and count
292-308: LGTM - basic tests cover BinlogBatch placeholder behavior.The tests verify:
- Default initialization with count = 0
- Commit correctly returns an error for the unimplemented batch
Additional tests for
RocksBatchwould require mocking theEnginetrait, which may be covered in integration tests.
Tsukikage7
force-pushed
the
feature/support-batch
branch
from
0c274a4 to
f1e29c6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/storage/src/redis_lists.rs (1)
657-727: Fix unused variablelists_data_cfto resolve pipeline failure.The variable is obtained via the macro but never used since
lsetnow uses the batch abstraction.🔧 Proposed fix
- let (db, cfs) = get_db_and_cfs!(self, ColumnFamilyIndex::ListsDataCF); - let lists_data_cf = &cfs[0]; + let (db, _cfs) = get_db_and_cfs!(self, ColumnFamilyIndex::ListsDataCF);Or if the macro is only needed for
db:- let (db, cfs) = get_db_and_cfs!(self, ColumnFamilyIndex::ListsDataCF); - let lists_data_cf = &cfs[0]; + let (db, _) = get_db_and_cfs!(self, ColumnFamilyIndex::ListsDataCF);
🤖 Fix all issues with AI agents
In @src/storage/src/redis_zsets.rs:
- Around line 1096-1102: The doc comment above the internal helper for
ZINTERSTORE and ZUNIONSTORE has a mis-indented list item causing a lint error;
adjust the doc comment so the numbered list items ("1. Collects members...", "2.
Deletes any existing destination data", "3. Writes new result data") are
indented as part of the doc paragraph (indent each list line by four spaces or
align them under the paragraph text) so the Rust doc linter recognizes them as a
list.
🧹 Nitpick comments (2)
src/storage/src/batch.rs (1)
287-303: Unit tests provide basic coverage for BinlogBatch.Consider adding tests for RocksBatch when integration test infrastructure is available, particularly for error cases like invalid CF handles.
src/storage/src/redis_strings.rs (1)
1106-1110: Unused variable_cfshould be removed.The
_cfvariable is obtained but never used since the batch abstraction handles CF selection internally.♻️ Proposed fix
- let _cf = self - .get_cf_handle(ColumnFamilyIndex::MetaCF) - .context(OptionNoneSnafu { - message: "cf is not initialized".to_string(), - })?;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/storage/src/batch.rssrc/storage/src/redis.rssrc/storage/src/redis_lists.rssrc/storage/src/redis_strings.rssrc/storage/src/redis_zsets.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/storage/src/redis.rs
🧰 Additional context used
🧬 Code graph analysis (4)
src/storage/src/redis_strings.rs (2)
src/storage/src/batch.rs (2)
new(117-141)new(242-244)src/engine/src/rocksdb_engine.rs (1)
db(38-40)
src/storage/src/redis_lists.rs (1)
src/storage/src/batch.rs (2)
new(117-141)new(242-244)
src/storage/src/redis_zsets.rs (1)
src/storage/src/batch.rs (3)
new(117-141)new(242-244)default(248-250)
src/storage/src/batch.rs (2)
src/storage/src/redis.rs (1)
get_cf_handle(313-323)src/engine/src/rocksdb_engine.rs (1)
db(38-40)
🪛 GitHub Actions: ci
src/storage/src/redis_lists.rs
[error] 664-664: unused variable: lists_data_cf during 'make lint' (rustc error) - Command failed with exit code 1.
src/storage/src/redis_zsets.rs
[error] 1291-1291: Formatting check failed: rustfmt suggested change at line 1291. Run 'make fmt-check' again after applying formatting changes (or run 'cargo fmt' / 'make fmt' to fix).
🪛 GitHub Actions: Tests
src/storage/src/redis_lists.rs
[error] 664-664: unused variable: lists_data_cf
🪛 GitHub Check: cargo clippy (ubuntu-latest)
src/storage/src/redis_zsets.rs
[failure] 1102-1102:
doc list item without indentation
🪛 GitHub Check: cargo fmt (ubuntu-latest)
src/storage/src/redis_zsets.rs
[warning] 1291-1291:
Diff in /home/runner/work/kiwi/kiwi/src/storage/src/redis_zsets.rs
🪛 GitHub Check: lint
src/storage/src/redis_zsets.rs
[failure] 1102-1102:
doc list item without indentation
🔇 Additional comments (24)
src/storage/src/batch.rs (5)
56-90: Well-designed trait abstraction for batch operations.The
Batchtrait properly abstracts the batch write mechanism with clear documentation. TheSendbound enables cross-thread usage, and returningResult<()>instead of panicking is appropriate for production storage systems.
106-142: Constructor validation is appropriate but consider using a debug assertion.The
assert_eq!will panic in production if there's a mismatch. This is documented as a programming error, but consider whether a fallible constructor returningResultmight be safer for library consumers.That said, since this is an internal invariant that should never be violated at runtime (it would indicate a code bug), the current approach is reasonable.
172-189: Good error handling for missing CF handles.The
get_cf_handlefunction properly returns a descriptive error with the CF index information, which will help debugging initialization issues.
191-220: RocksBatch implementation is correct and straightforward.The implementation properly delegates to RocksDB's
WriteBatchand tracks count internally. Thecommitmethod correctly uses theBox<Self>pattern to consume the batch.
253-285: BinlogBatch placeholder correctly prevents silent data loss.The
commitmethod returning an error rather than silently succeeding is the right approach for an unimplemented feature. This ensures callers will be aware that cluster mode isn't ready.src/storage/src/redis_strings.rs (6)
318-324: Correct batch-based write for setrange.The migration to
create_batch()withColumnFamilyIndex::MetaCFfollows the new pattern correctly.
389-395: Correct batch-based write for append.Consistent with the new batch abstraction pattern.
581-587: Correct batch-based write for set.Simple and correct migration.
1063-1078: mset correctly uses batch for atomic multi-key operation.The batch is created once and all key-value pairs are added before a single commit, ensuring atomicity.
2083-2162: del_key correctly collects keys before batch deletion to avoid iterator invalidation.The approach of collecting keys to delete first, then performing batch deletions, is correct and avoids modifying the iterator while iterating. The prefix-based scan for data CFs properly handles composite data types.
2233-2285: flush_db chunked deletion is well-designed for large databases.The 1000-key chunk size is reasonable. Creating a new batch per chunk ensures memory is released between iterations. This prevents OOM issues on large databases.
src/storage/src/redis_zsets.rs (6)
30-33: Unused import removed appropriately.The
WriteBatchimport was removed as it's no longer directly used, replaced by the batch abstraction.
117-144: Good floating-point score comparison using epsilon.Using
(existing_score - sm.score).abs() < f64::EPSILONis the correct approach for comparing floating-point numbers for equality.
99-181: zadd correctly updates both data and score CFs atomically.The batch properly handles member updates across
ZsetsDataCFandZsetsScoreCF, including deleting old score keys when scores change.
1222-1313: zset_store_operation atomicity is correctly implemented.The function properly:
- Collects members from source zsets
- Acquires lock on destination
- Deletes existing destination data in the same batch
- Writes new result data
- Commits all in one batch
This ensures atomic replacement of destination zset.
826-878: zrem correctly handles empty zset cleanup.When all members are removed, the meta key is deleted. Otherwise, the count is updated. This is consistent with Redis behavior.
1286-1291: No formatting issue detected at lines 1286-1291.The code at these lines is properly formatted with consistent indentation and correct Rust conventions. Line 1291 is blank, and the
batch.put()call (lines 1286-1290) follows standard formatting patterns. The formatting does not require changes.src/storage/src/redis_lists.rs (7)
105-107: Good refactoring to collect batch operations before commit.Using vectors to collect
putsanddeletesbefore applying them in a single batch is a clean pattern that improves readability and ensures atomicity.
236-245: Batch commit correctly applies deletes before puts.The order of operations (deletes first, then puts) is appropriate for reorganizing list elements when pushing to the head.
341-353: Correct empty list cleanup in lpop.Deleting the meta key when the list becomes empty ensures
EXISTSreturns false andLPUSHX/RPUSHXbehave correctly per Redis protocol.
720-727: lset correctly uses batch for consistency with cluster mode.Even for single-element updates, using the batch abstraction ensures future cluster-mode compatibility.
774-776: Good use of boolean flags to track batch operations.Using
delete_metaandupdate_metaflags to determine which operations to include in the batch is clear and maintainable.
814-825: ltrim batch commit handles both delete and update cases correctly.The conditional logic properly handles the case where the entire list is deleted vs. when it's just trimmed.
1100-1173: rpoplpush atomicity is maintained through proper locking.The implementation correctly:
- Handles same-key case with single lock
- Acquires locks in sorted order for different keys to prevent deadlocks
- Uses internal methods to avoid double locking
- Fix zadd score equality check bug (comparing difference instead of just sm.score) - Fix lset to use batch layer instead of direct db.put_cf() - Fix zunionstore/zinterstore atomicity by merging delete and write into single batch commit - Improve EXPECTED_CF_COUNT by deriving from ColumnFamilyIndex::COUNT - Fix del_key prefix scan to use correct data key prefix format - Optimize flush_db with chunked deletion (1000 keys per batch) to avoid memory issues with large databases
Tsukikage7
force-pushed
the
feature/support-batch
branch
from
f1e29c6 to
f76ba07
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/storage/src/batch.rs (1)
144-189: Consider deriving index from enum definition to reduce maintenance burden.The explicit
matchincf_index_to_usizeensures compile-time exhaustiveness, which is good. However, the hardcoded indices (0-5) must stay in sync with how column family handles are populated inRedis::create_batch(). If the order changes, this could cause subtle bugs.💡 Alternative: Derive index from enum
If
ColumnFamilyIndexderives a trait likenum_enum::IntoPrimitiveor has explicit discriminants, you could usecf_idx as usizedirectly. This would eliminate the need to manually maintain the mapping:// If ColumnFamilyIndex has explicit discriminants: #[repr(usize)] pub enum ColumnFamilyIndex { MetaCF = 0, HashesDataCF = 1, // ... } #[inline] fn cf_index_to_usize(cf_idx: ColumnFamilyIndex) -> usize { cf_idx as usize }src/storage/src/redis_strings.rs (1)
1106-1110: Remove unused_cfvariable.The
_cfvariable is fetched but never used. This appears to be dead code from before the refactoring.🧹 Remove unused variable
pub fn msetnx(&self, kvs: &[(Vec<u8>, Vec<u8>)]) -> Result<bool> { let db = self.db.as_ref().context(OptionNoneSnafu { message: "db is not initialized".to_string(), })?; - let _cf = self - .get_cf_handle(ColumnFamilyIndex::MetaCF) - .context(OptionNoneSnafu { - message: "cf is not initialized".to_string(), - })?; - // Check if any key exists and is not expired
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/storage/src/batch.rssrc/storage/src/redis.rssrc/storage/src/redis_lists.rssrc/storage/src/redis_strings.rssrc/storage/src/redis_zsets.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/storage/src/redis.rs
🧰 Additional context used
🧬 Code graph analysis (4)
src/storage/src/redis_lists.rs (2)
src/storage/src/batch.rs (2)
new(117-141)new(242-244)src/storage/src/redis.rs (1)
new(120-154)
src/storage/src/redis_zsets.rs (1)
src/storage/src/batch.rs (3)
new(117-141)new(242-244)default(248-250)
src/storage/src/redis_strings.rs (2)
src/storage/src/batch.rs (2)
new(117-141)new(242-244)src/storage/src/storage_define.rs (1)
encode_user_key(49-68)
src/storage/src/batch.rs (2)
src/storage/src/redis.rs (1)
get_cf_handle(313-323)src/engine/src/rocksdb_engine.rs (1)
db(38-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: test-macos
- GitHub Check: test-windows
- GitHub Check: test
- GitHub Check: build and test (macos-latest)
- GitHub Check: cargo clippy (macos-latest)
- GitHub Check: cargo clippy (windows-latest)
- GitHub Check: build and test (ubuntu-latest)
- GitHub Check: build and test (windows-latest)
🔇 Additional comments (34)
src/storage/src/batch.rs (6)
1-46: LGTM! Well-structured module header and imports.The module documentation clearly explains the design intent for standalone vs. cluster modes, and the usage example is helpful. The imports are minimal and appropriate.
47-90: LGTM! Well-designed trait with appropriate ownership semantics.The
commit(self: Box<Self>)signature correctly consumes the batch to prevent reuse after commit. TheSendbound enables usage in async contexts. Error handling viaResult<()>is consistent across all operations.
191-220: LGTM! Clean Batch implementation for RocksBatch.The implementation correctly:
- Validates CF handles before operations and propagates errors
- Tracks operation count only after successful operations
- Delegates commit to the underlying engine with proper error context
- Resets both internal state and count on clear
222-285: LGTM! BinlogBatch placeholder correctly fails on commit.The design appropriately prevents silent data loss by returning an error on
commit(). Theput/deleteoperations succeed (incrementing count) but the finalcommitwill fail - this is acceptable for a placeholder since:
- It allows code paths to be tested without cluster mode
- It fails loudly at commit time rather than silently losing data
The TODO comments clearly document what needs to be implemented for Raft integration.
287-303: LGTM! Basic test coverage for BinlogBatch.The tests verify the essential behavior: default state and commit failure. Additional tests for
put/deleteincrementingcountcould be added, but the current coverage is sufficient for the placeholder implementation.
92-142: LGTM! RocksBatch construction with appropriate validation.The
assert_eqforcf_handles.len()vsColumnFamilyIndex::COUNTis a good defensive programming practice to catch configuration mismatches early. The panic is acceptable here since it indicates a programming error that should be caught during development/testing.ColumnFamilyIndex::COUNTis correctly defined as 6, matching the 6 column family variants (MetaCF, HashesDataCF, SetsDataCF, ListsDataCF, ZsetsDataCF, ZsetsScoreCF).src/storage/src/redis_zsets.rs (9)
30-30: LGTM! Import cleanup reflects the batch abstraction migration.Removing
WriteBatchandWriteOptionsimports is correct since batch operations are now abstracted through theBatchtrait.
99-181: LGTM! zadd correctly uses batch abstraction for atomic updates.The refactoring properly:
- Creates batch via
self.create_batch()?- Uses
ColumnFamilyIndexenums for type-safe CF access- Handles both existing zset updates and new zset creation
- Commits batch atomically at the end of each code path
383-411: LGTM! zincrby correctly batches score update operations.The batch properly groups the old score key deletion with new score/member key insertions for atomic update. The delegation to
zadd()for new members is appropriate sincezaddhandles its own batch.
826-878: LGTM! zrem correctly batches member deletions and meta updates.The implementation properly:
- Batches all score and member key deletions
- Handles the empty zset case by deleting the meta key
- Updates meta count when members remain
- Commits atomically
1096-1103: LGTM! Well-documented atomic batch operation.The updated docstring clearly explains the three-phase operation (collect, delete, write) and emphasizes the atomicity guarantee provided by the batch.
1223-1314: LGTM! zset_store_operation correctly implements atomic destination update.The refactored code properly:
- Acquires lock on destination before any modifications
- Creates a single batch for all destination changes
- Deletes existing destination data (if any) within the batch
- Writes new result data within the same batch
- Commits atomically, ensuring no partial state is visible
This is a significant improvement over the previous implementation as it guarantees atomicity for the entire destination update.
1585-1607: LGTM! zremrangebylex batch changes follow consistent pattern.The implementation correctly batches member/score deletions with meta updates, maintaining the same pattern as other zrem* operations.
1708-1730: LGTM! zremrangebyrank batch changes are consistent.
1811-1833: LGTM! zremrangebyscore batch changes follow the established pattern.src/storage/src/redis_strings.rs (10)
318-324: LGTM! setrange correctly uses batch for single-key write.Simple and correct migration to batch abstraction.
389-395: LGTM! append batch changes follow consistent pattern.
581-587: LGTM! set, setex, psetex batch changes are consistent.All three functions follow the same pattern for atomic writes with TTL handling.
759-765: LGTM! setnx and getset batch changes are consistent.
1063-1078: LGTM! mset correctly batches multiple key-value writes atomically.The batch ensures all keys are written atomically as required by Redis MSET semantics.
1212-1218: LGTM! incr_decr and incr_decr_float batch changes are consistent.
1429-1435: LGTM! setbit batch changes follow consistent pattern.
1836-1843: LGTM! bitop correctly uses batch for each execution path.Multiple batch creation points are appropriate here since each code path has a single write operation followed by early return.
2083-2162: LGTM! del_key refactored for atomic batch deletion with proper prefix scanning.The implementation correctly:
- Builds the data key prefix using
encode_user_keyto match the data key format- Collects all keys to delete first (avoiding borrow conflicts)
- Iterates over all relevant column families (Hashes, Sets, Lists, Zsets data and score)
- Batches all deletions atomically
The comment on lines 2113-2117 explains the key format well, which is helpful for maintainability.
2233-2280: LGTM! flush_db uses chunked deletion to handle large databases.The chunked approach with
CHUNK_SIZE = 1000is appropriate for flush operations:
- Prevents memory exhaustion on large databases
- Makes incremental progress (earlier chunks persist even if later ones fail)
- Commits each chunk separately, which is acceptable for a destructive operation like flush
Note: This is intentionally not fully atomic - if a failure occurs mid-flush, some data will already be deleted. This is acceptable behavior for
FLUSHDB.src/storage/src/redis_lists.rs (9)
105-107: LGTM! push_core correctly collects operations before batching.The two-phase approach (collect in
puts/deletesvectors, then batch and commit) avoids borrow conflicts and ensures all operations are committed atomically.
236-245: LGTM! Batch commit order is correct.The ordering (deletes → puts → meta update → commit) ensures:
- Old data is removed before new data is written
- Metadata is updated after data operations
- All changes are committed atomically
341-353: LGTM! lpop correctly handles empty list case.The batch properly:
- Deletes popped element keys
- Deletes meta key if list becomes empty (consistent with Redis behavior)
- Updates meta key otherwise
- Commits atomically
417-429: LGTM! rpop_internal follows consistent pattern.
493-505: LGTM! rpop follows consistent pattern.
719-726: LGTM! lset correctly batches single element update.
813-824: LGTM! ltrim correctly handles delete-all vs trim-range cases.The use of
delete_metaandupdate_metaflags cleanly separates the two cases:
- Delete all elements → delete meta key
- Trim to range → update meta key with new indices
942-955: LGTM! lrem correctly batches element removal and rewrite.The implementation properly:
- Deletes all existing elements
- Writes back remaining elements
- Handles empty list case by deleting meta
1082-1091: LGTM! linsert correctly batches element rewrite after insertion.
2 participants
关联 Issue
Closes #222
背景
当前存储层直接使用 RocksDB 的 WriteBatch 进行批量写操作，这种实现方式与底层存储引擎强耦合，无法支持后续的集群模式（需要通过 Raft 共识进行写入）。
解决方案
引入 Batch trait 抽象层，将批量写操作从具体实现中解耦：
主要改动
batch.rs模块，包含 Batch trait 及两种实现create_batch()工厂方法redis_strings.rs(2 处)redis_hashes.rs(6 处)redis_lists.rs(7 处)redis_sets.rs(6 处)redis_zsets.rs(8 处)测试情况
Checklist
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.