Skip to content

Conversation

patrick-ogrady
Copy link
Contributor

@patrick-ogrady patrick-ogrady commented Jun 29, 2025

Related: #1135

Changes

  • Add concurrent writes during sync

@patrick-ogrady patrick-ogrady marked this pull request as ready for review June 30, 2025 01:10
@patrick-ogrady patrick-ogrady requested a review from Copilot June 30, 2025 01:30
Copilot

This comment was marked as outdated.

@patrick-ogrady patrick-ogrady changed the title [storage] Introduce ordinal [storage] Introduce ordinal + Ensure destroy() Deletes Entire Partition Jun 30, 2025
@patrick-ogrady patrick-ogrady marked this pull request as draft June 30, 2025 17:03
@patrick-ogrady patrick-ogrady requested a review from Copilot July 1, 2025 20:45
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces a new ordinal storage engine for sparse, fixed‐size records and updates existing storage modules so that calling destroy() removes the entire partition. It also adds benchmark support for the new engine.

  • Add ordinal module with an Ordinal type supporting put/get/has/prune/sync/close/destroy
  • Ensure destroy() removes the entire partition in ordinal, metadata, and both journal implementations
  • Wire up benchmarks for ordinal in Cargo.toml and add bench code under storage/src/ordinal/benches

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
storage/src/ordinal/storage.rs New Ordinal implementation with full lifecycle and partition destroy
storage/src/ordinal/mod.rs Module docs, type exports, config and error definitions
storage/src/ordinal/benches/utils.rs Benchmark helpers for ordinal
storage/src/ordinal/benches/restart.rs Restart benchmark
storage/src/ordinal/benches/put.rs Put benchmark
storage/src/ordinal/benches/get.rs Get benchmark
storage/src/ordinal/benches/bench.rs Criterion runner setup
storage/src/metadata/storage.rs Added partition-wide remove in metadata's destroy
storage/src/journal/variable.rs Added partition-wide remove in variable-journal's destroy
storage/src/journal/fixed.rs Added partition-wide remove in fixed-journal's destroy
storage/src/lib.rs Exposed new ordinal module
storage/Cargo.toml Registered ordinal benchmarks
Comments suppressed due to low confidence (3)

storage/src/ordinal/mod.rs:80

  • The example calls the async put method without .await. Update to store.put(0, value1).await.unwrap(); (and similarly for the other put calls) so the snippet compiles correctly.
//!     store.put(0, value1).unwrap();

storage/src/journal/variable.rs:682

  • The code references RError but there’s no use commonware_runtime::Error as RError; import in this file, causing a compilation error. Please add the import alongside other commonware_runtime imports.
            Err(RError::PartitionMissing(_)) => {

storage/src/journal/fixed.rs:520

  • This block uses RError::PartitionMissing but the alias RError isn’t imported here. Add use commonware_runtime::Error as RError; with the other imports.
        match self.context.remove(&self.cfg.partition, None).await {

@patrick-ogrady patrick-ogrady marked this pull request as ready for review July 1, 2025 20:52
@patrick-ogrady patrick-ogrady merged commit ba2a872 into main Jul 1, 2025
29 checks passed
@patrick-ogrady patrick-ogrady deleted the ordinal branch July 1, 2025 21:05
Copy link

codecov bot commented Jul 1, 2025

Codecov Report

Attention: Patch coverage is 97.29494% with 31 lines in your changes missing coverage. Please review.

Project coverage is 91.40%. Comparing base (e9e0731) to head (0d6e4da).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
storage/src/ordinal/storage.rs 91.82% 17 Missing ⚠️
storage/src/journal/variable.rs 0.00% 6 Missing ⚠️
storage/src/journal/fixed.rs 33.33% 4 Missing ⚠️
storage/src/metadata/storage.rs 33.33% 4 Missing ⚠️
@@            Coverage Diff             @@
##             main    #1173      +/-   ##
==========================================
+ Coverage   91.28%   91.40%   +0.11%     
==========================================
  Files         216      218       +2     
  Lines       58364    59510    +1146     
==========================================
+ Hits        53278    54393    +1115     
- Misses       5086     5117      +31     
Files with missing lines Coverage Δ
storage/src/ordinal/mod.rs 100.00% <100.00%> (ø)
storage/src/journal/fixed.rs 97.67% <33.33%> (-0.46%) ⬇️
storage/src/metadata/storage.rs 97.50% <33.33%> (-1.41%) ⬇️
storage/src/journal/variable.rs 92.25% <0.00%> (-0.51%) ⬇️
storage/src/ordinal/storage.rs 91.82% <91.82%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9e0731...0d6e4da. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant