feat: add partition-aware storage trait abstraction#49
Closed
vieiralucas wants to merge 5 commits intomainfrom
Closed
feat: add partition-aware storage trait abstraction#49vieiralucas wants to merge 5 commits intomainfrom
vieiralucas wants to merge 5 commits intomainfrom
Conversation
Add PartitionId type with DEFAULT constant for single-node mode. All Storage trait methods now accept a partition parameter, enabling future multi-partition clustering. Rename StorageError::RocksDb to Backend for backend-agnosticism. Update all scheduler call sites, Lua bridge, and tests to pass PartitionId::DEFAULT.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 24 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="_bmad-output/implementation-artifacts/13-1-storage-trait-abstraction-rocksdb-adapter.md">
<violation number="1" location="_bmad-output/implementation-artifacts/13-1-storage-trait-abstraction-rocksdb-adapter.md:31">
P2: Acceptance Criteria #2 and #5 contradict the actual implementation described in the Tasks and Completion Notes sections of this same document. AC #2 says `WriteBatchOp` is "extended to carry partition context" (it wasn't—partition is passed at `write_batch()` call-site instead), and AC #5 says `keys.rs` functions accept a partition parameter (Task 3 explicitly marks this as a design deviation that was *not* done). Update the ACs to reflect the actual design decisions, or add clear "Not Met / Deferred" annotations, so someone checking story completion against ACs isn't misled.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| @@ -0,0 +1,229 @@ | |||
| # Story 13.1: Storage Trait Abstraction & RocksDB Adapter | |||
There was a problem hiding this comment.
P2: Acceptance Criteria #2 and #5 contradict the actual implementation described in the Tasks and Completion Notes sections of this same document. AC #2 says WriteBatchOp is "extended to carry partition context" (it wasn't—partition is passed at write_batch() call-site instead), and AC #5 says keys.rs functions accept a partition parameter (Task 3 explicitly marks this as a design deviation that was not done). Update the ACs to reflect the actual design decisions, or add clear "Not Met / Deferred" annotations, so someone checking story completion against ACs isn't misled.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At _bmad-output/implementation-artifacts/13-1-storage-trait-abstraction-rocksdb-adapter.md, line 31:
<comment>Acceptance Criteria #2 and #5 contradict the actual implementation described in the Tasks and Completion Notes sections of this same document. AC #2 says `WriteBatchOp` is "extended to carry partition context" (it wasn't—partition is passed at `write_batch()` call-site instead), and AC #5 says `keys.rs` functions accept a partition parameter (Task 3 explicitly marks this as a design deviation that was *not* done). Update the ACs to reflect the actual design decisions, or add clear "Not Met / Deferred" annotations, so someone checking story completion against ACs isn't misled.</comment>
<file context>
@@ -0,0 +1,229 @@
+ **And** a `PartitionId` type is defined with a `DEFAULT` constant for single-partition mode
+ **And** the default partition preserves current key encoding behavior exactly
+
+2. **Given** the existing `RocksDbStorage` at `crates/fila-core/src/storage/rocksdb.rs`
+ **When** it implements the partition-aware `Storage` trait
+ **Then** the default partition maps to the current column family key encoding unchanged
</file context>
Contributor
Benchmark Results (median of 3 runs)Commit:
|
Contributor
Benchmark Results (median of 3 runs)Commit:
|
Contributor
Benchmark Results (median of 3 runs)Commit:
|
Member
Author
|
Closing: Epic 13 reshaped per clustering architecture research. The Raft-per-queue model (see _bmad/docs/research/decoupled-scheduler-sharded-storage.md) makes the purpose-built storage engine redundant — RocksDB serves as the Raft state machine backend. Epic 13 is being replaced with a 2-story storage trait cleanup + clustering prep epic. |
vieiralucas
added a commit
that referenced
this pull request
Mar 11, 2026
…r-queue model technical research validated cockroachdb-style single-binary raft-per-queue clustering architecture. this makes the purpose-built storage engine (epic 13) redundant and the partition-based clustering (epic 14) incompatible. changes: - close prs #49-53 (epic 13 storage engine, none merged) - reshape epic 13: 5 stories → 2 (storage trait cleanup + phase 2 viability seams) - replan epic 14: partition-based → raft-per-queue (5 stories redesigned) - defer fr66 (custom storage engine) and nfr30-33 to post-clustering - update prd, architecture doc, epics, sprint-status
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PartitionIdnewtype (u32withDEFAULTconstant) for partition-namespace awareness in preparation for multi-partition clustering (Epic 14)Storagetrait: all methods now acceptpartition: &PartitionIdparameter (exceptflush())StorageError::RocksDb→StorageError::Backendfor backend-agnosticismPartitionId::DEFAULTDesign Decisions
write_batch()call site, not perWriteBatchOpvariant — cleaner APIkeys.rsnot updated with partition parameter — partition handled at Storage trait level (key namespacing deferred to Story 13.2+ when custom storage engine is built)delivery.rsuses&self.partitiondirect field access instead ofself.p()to avoid borrow checker conflict withself.consumer_rr_idxTest plan
cargo test --workspace— 294 tests passcargo clippy --workspace --all-targets— clean🤖 Generated with Claude Code
Summary by cubic
Introduces partition-aware storage by adding
PartitionIdand updating theStoragetrait to require a partition for all operations. RocksDB, scheduler, and Lua run on the default partition so behavior stays the same, satisfying Story 13.1 (Epic 13) and preparing for multi-partition clustering.New Features
PartitionId(u32)withDEFAULT; allStoragemethods now take&PartitionId(exceptflush()).StorageError::RocksDbtoStorageError::Backend.write_batch()call site (no per-variant partition).Migration
Storageimplementors to new method signatures.PartitionId::DEFAULTat call sites (scheduler, Lua, tests).LuaEngine::new()now takes aPartitionId; passPartitionId::DEFAULT.StorageError::RocksDbwithStorageError::Backend.Written for commit f3c67ae. Summary will update on new commits.