[storage] Add index to Archive
#176
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several key changes to the storage module, including the addition of a new dependency ( Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
patrick-ogrady
changed the title
Indexindex to Archive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
storage/Cargo.toml(1 hunks)storage/src/archive/mod.rs(27 hunks)storage/src/archive/storage.rs(11 hunks)storage/src/journal/mod.rs(2 hunks)storage/src/journal/storage.rs(1 hunks)
🧰 Additional context used
📓 Learnings (1)
storage/src/journal/storage.rs (1)
Learnt from: patrick-ogrady
PR: commonwarexyz/monorepo#161
File: storage/src/journal/storage.rs:307-310
Timestamp: 2024-11-04T17:31:58.490Z
Learning: The test `test_journal_read_checksum_missing` in `storage/src/journal/mod.rs` covers the code handling blob truncation when trailing bytes are detected.
🪛 GitHub Check: codecov/patch
storage/src/archive/mod.rs
[warning] 269-269: storage/src/archive/mod.rs#L269
Added line #L269 was not covered by tests
[warning] 274-274: storage/src/archive/mod.rs#L274
Added line #L274 was not covered by tests
[warning] 287-287: storage/src/archive/mod.rs#L287
Added line #L287 was not covered by tests
[warning] 292-292: storage/src/archive/mod.rs#L292
Added line #L292 was not covered by tests
[warning] 299-299: storage/src/archive/mod.rs#L299
Added line #L299 was not covered by tests
[warning] 411-411: storage/src/archive/mod.rs#L411
Added line #L411 was not covered by tests
[warning] 620-620: storage/src/archive/mod.rs#L620
Added line #L620 was not covered by tests
[warning] 677-677: storage/src/archive/mod.rs#L677
Added line #L677 was not covered by tests
[warning] 685-685: storage/src/archive/mod.rs#L685
Added line #L685 was not covered by tests
[warning] 998-998: storage/src/archive/mod.rs#L998
Added line #L998 was not covered by tests
[warning] 1048-1048: storage/src/archive/mod.rs#L1048
Added line #L1048 was not covered by tests
[warning] 1083-1083: storage/src/archive/mod.rs#L1083
Added line #L1083 was not covered by tests
storage/src/archive/storage.rs
[warning] 207-207: storage/src/archive/storage.rs#L207
Added line #L207 was not covered by tests
[warning] 235-236: storage/src/archive/storage.rs#L235-L236
Added lines #L235 - L236 were not covered by tests
[warning] 247-247: storage/src/archive/storage.rs#L247
Added line #L247 was not covered by tests
[warning] 378-378: storage/src/archive/storage.rs#L378
Added line #L378 was not covered by tests
[warning] 489-489: storage/src/archive/storage.rs#L489
Added line #L489 was not covered by tests
[warning] 511-511: storage/src/archive/storage.rs#L511
Added line #L511 was not covered by tests
[warning] 553-553: storage/src/archive/storage.rs#L553
Added line #L553 was not covered by tests
[warning] 560-560: storage/src/archive/storage.rs#L560
Added line #L560 was not covered by tests
🔇 Additional comments (5)
storage/Cargo.toml (1)
25-25: Verify rangemap dependency
✅ Verification successful
Dependency verified and good to go
- Version 1.5.1 is the latest available
- License (MIT/Apache-2.0) is compatible with the project
- Properly used via
RangeInclusiveSetin storage implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check latest version and license of rangemap
curl -s https://crates.io/api/v1/crates/rangemap | jq -r '.crate | "Latest version: " + .max_version + "\nLicense: " + .license'
# Verify actual usage of rangemap in the codebase
rg -l "use.*rangemap"
rg -l "::rangemap"
Length of output: 236
Script:
#!/bin/bash
# Check rangemap's license from its repository
curl -s https://raw.githubusercontent.com/jeffparsons/rangemap/master/Cargo.toml | rg -A 1 'license'
# Check actual usage context
rg -A 5 -B 5 'rangemap' storage/src/archive/storage.rs
# Check if rangemap is actually used in the implementation
ast-grep --pattern 'use rangemap::$_'
Length of output: 710
storage/src/journal/mod.rs (1)
145-145: LGTM!
The addition of BufMut import is appropriate for the buffer manipulation needs.
storage/src/journal/storage.rs (1)
271-288: 🧹 Nitpick (assertive)
🛠️ Refactor suggestion
Add tests for new blob boundary conditions
Ensure the new checks for end-of-blob and over-read conditions in the replay method are covered by unit tests to prevent regressions.
Do you want assistance in creating these tests or opening a GitHub issue to track this?
⛔ Skipped due to learnings
Learnt from: patrick-ogrady
PR: commonwarexyz/monorepo#161
File: storage/src/journal/storage.rs:307-310
Timestamp: 2024-11-04T17:31:58.490Z
Learning: The test `test_journal_read_checksum_missing` in `storage/src/journal/mod.rs` covers the code handling blob truncation when trailing bytes are detected.
storage/src/archive/storage.rs (2)
378-379: Unnecessary Use of .ok_or After a Non-Fallible Call
The call to .get on the journal should always return Some if indices contains the key. The use of .ok_or(Error::RecordCorrupted)? suggests that None is possible, which may not be the case. Verify whether this check is necessary or if it can be removed to simplify the code.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 378-378: storage/src/archive/storage.rs#L378
Added line #L378 was not covered by tests
445-452: 🧹 Nitpick (assertive)
Simplify the has Method Logic
The has method can be streamlined by directly returning the result of the checks without matching on Identifier unnecessarily.
Apply this diff for a cleaner implementation:
pub async fn has(&self, identifier: Identifier<'_>) -> Result<bool, Error> {
self.has.inc();
match identifier {
- Identifier::Index(index) => Ok(self.has_index(index)),
- Identifier::Key(key) => self.has_key(key).await,
+ Identifier::Index(index) => Ok(self.indices.contains_key(&index)),
+ Identifier::Key(key) => {
+ let result = self.get_key(key).await?.is_some();
+ Ok(result)
+ }
}
}Likely invalid or redundant comment.
| Some(prefix) => Self::read_prefix(blob, offset, prefix).await, | ||
| None => Self::read(blob, offset).await, | ||
| }; | ||
|
|
||
| // Ensure a full read wouldn't put us past the end of the blob |
There was a problem hiding this comment.
This was a bug.
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #176 +/- ##
==========================================
+ Coverage 92.96% 93.09% +0.13%
==========================================
Files 56 56
Lines 11320 11505 +185
==========================================
+ Hits 10524 10711 +187
+ Misses 796 794 -2
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Related: #175 (Finalized Height <> Block index)
put(also update Journal)indextoputand infersectionIntervalTreerangemap)?Summary by CodeRabbit
New Features
Identifiertype.Bug Fixes
Documentation