[storage/adb/any] anydb code cleanup #1107
Conversation
8a78e4e to
895199c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
7926feb to
699d315
Compare
699d315 to
ffdc09a
Compare
3f77e28 to
ff1e313
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors delete and update replay logic to reduce duplication, simplifies pattern matching, and aligns update handling with other operations.
- Extracts common delete logic into a new
delete_keyhelper and updates initialization replay to use it - Replaces two-arm
matchstatements withlet ... elseorif letfor more concise control flow - Updates the in-snapshot update logic to directly destructure
Operation::Updateand adjust cursor updates
Comments suppressed due to low confidence (3)
storage/src/adb/any.rs:414
- [nitpick] The variable name
ris ambiguous; consider renaming it to something more descriptive likedeleted_locorprev_locationto clarify its purpose.
let r = Self::delete_key(&mut self.snapshot, &self.log, &key).await?;
storage/src/adb/any.rs:424
- Add unit tests for
delete_keycovering both cases (key present vs. absent) and verify that the snapshot and bitmap updates behave as expected after refactoring.
async fn delete_key(
storage/src/adb/any.rs:259
- [nitpick] This binding shadows the outer
bitmapvariable, which can be confusing; consider renaming the inner binding (e.g.,if let Some(bm) = &mut bitmap) to improve readability.
if let Some(ref mut bitmap) = bitmap {
d9cb9b4 to
eafce72
Compare
9acce86 to
b05b23e
Compare
86a77c0 to
0fc44b7
Compare
storage/src/adb/any.rs
Outdated
| /// | ||
| /// # Warning | ||
| /// | ||
| /// Panics if the location does not reference an update operation. |
There was a problem hiding this comment.
nit: add a bit more context that this should not happen if the snapshot is correctly managed? Maybe that's overkill...
There was a problem hiding this comment.
no makes sense. done
storage/src/adb/any.rs
Outdated
| } | ||
| cursor.delete(); | ||
| break; | ||
| let delete_result = |
There was a problem hiding this comment.
nit: I think you can just call this result
There was a problem hiding this comment.
done. also renamed the "update_result" below to just "result"
0fc44b7 to
f9f666e
Compare
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #1107 +/- ##
==========================================
+ Coverage 90.99% 91.01% +0.01%
==========================================
Files 191 191
Lines 55227 55217 -10
==========================================
- Hits 50255 50253 -2
+ Misses 4972 4964 -8
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Makes initialization re-use delete() code when replaying delete operations to reduce code duplication.
Replace 2-arm match clauses with let/else.
Add additional assertion checking ensuring data consistency.
Better document invariants.