[NBS] issue-6279: persist blob metas in local db#6559
Conversation
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-relwithdebinfo have completed. Tip Planned checks for linux-x86_64-relwithdebinfo. 🟢 linux-x86_64-relwithdebinfo target: cloud/tasks/,cloud/storage/ (test time: 259s): all tests PASSED for commit f7c7b9a.
🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 274s): all tests PASSED for commit f7c7b9a.
🟢 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 1370s): all tests PASSED for commit f7c7b9a.
🟢 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 4972s): all tests PASSED for commit f7c7b9a.
|
vladstepanyuk
added
large-tests
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-relwithdebinfo have completed. Tip Planned checks for linux-x86_64-relwithdebinfo.
|
![robot-vibe-db[bot]](https://avatars.githubusercontent.com/in/3319454?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review Summary
Verdict: ✅ No critical issues found
Critical issues
No critical issues found.
Other findings
- Minor | Medium: In-memory cleanup queue items in test setup lack AdditionalFields despite DB having them, diverging from the production load path —
part_cleanup_logic_ut.cpp:162 - Minor | Low: Empty AdditionalFields proto written to DB when neither BlobMeta nor RecreatedBlobMeta is available for an affected blob —
part_actor_addblobs.cpp:614 - Minor | Medium: No test for backward-compatible upgrade path (reading old cleanup queue rows without the AdditionalFields column) —
part_database_ut.cpp:816 - Nit | High: Unnecessary blank line after
ifguard —part_actor_addblobs.cpp:613
This review was generated automatically. Critical issues require attention; other findings are advisory.
If this comment was useful, please give it a 👍 — it helps us improve the review bot.
| MakeCleanupQueueAdditionalFields(setup.MergedBlobMeta)); | ||
| }); | ||
|
|
||
| state.GetCleanupQueue().Add({setup.MixedBlobId, deletionCommitId, {}}); |
There was a problem hiding this comment.
Severity: Minor
Confidence: Medium
The in-memory queue is populated with empty {} AdditionalFields, while the DB (written in SetupMixedAndMergedBlobs) stores them with blob metas. In production, after partition restart, ReadCleanupQueue reads AdditionalFields from DB and populates the in-memory items with them via State->GetCleanupQueue().Add(args.CleanupQueue).
This setup intentionally tests the fallback path (no recreated blob meta), but the divergence from the production load path could confuse future readers. Consider adding a brief comment here explaining why AdditionalFields is intentionally left empty (e.g., "// Intentionally empty to test the fallback path without recreated blob meta").
| if (IsBlockMaskFull(blockMask, MaxBlocksInBlob)) { | ||
| NProto::TBlobMeta blobMeta; | ||
|
|
||
| NProto::TCleanupQueueAdditionalFields additionalFields; |
There was a problem hiding this comment.
Severity: Minor
Confidence: Low
When neither kv.second.BlobMeta nor kv.second.RecreatedBlobMeta is available, an empty TCleanupQueueAdditionalFields proto is constructed and written to the DB. This is functionally correct (the cleanup logic falls through to reading blob meta from DB), but it stores unnecessary empty data in the cleanup queue table.
Consider wrapping the DB write and in-memory add logic so that the AdditionalFields is only populated when there's actually meta to store, or accept the minor overhead as a design simplification.
| additionalFields.SerializeAsString(), | ||
| items[0].AdditionalFields.SerializeAsString()); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Severity: Minor
Confidence: Medium
This test verifies the write/read roundtrip with AdditionalFields, which is good. Consider adding a companion test that verifies backward compatibility with old-format cleanup queue rows. Specifically:
- Write a cleanup queue entry using the old format (without the AdditionalFields column, or with only key columns)
- Read it back and verify that
AdditionalFieldsis default-constructed (empty) - Verify that the cleanup logic handles this empty case correctly (falls through to DB read)
While GetValueOrDefault handles missing columns automatically, an explicit test would document and protect this upgrade path, especially since existing partitions will have old-format rows when this code is deployed.
|
|
||
| if (IsBlockMaskFull(blockMask, MaxBlocksInBlob)) { | ||
| NProto::TBlobMeta blobMeta; | ||
|
|
There was a problem hiding this comment.
Severity: Nit
Confidence: High
Unnecessary blank line after the if guard. The surrounding code style in this file doesn't use blank lines after if guards.
|
Analysis performed by claude, claude-opus-4-6. |
1 participant
Notes
Added persisting blob metas from cleanup queue in local db.
Issue
#6279