[fix](cloud) recycler decrement_delete_bitmap_packed_file_ref_counts method should read delete bitmaps as blobs - #66728
Conversation
### What problem does this PR solve? Issue Number: close #xxx Related PR: #xxx Problem Summary: Versioned delete bitmaps are written with blob_put, which appends a suffix to every physical key. The recycler read the unsuffixed logical key with Transaction::get, treated existing delete bitmap metadata as missing, and skipped standalone-file deletion or packed-file reference-count updates. Read the logical value with blob_get and deserialize it from ValueBuf. Update the recycler test fixture to use the production blob encoding so existing rowset recycling tests cover the failure. ### Release note None ### Check List (For Author) - Test: Unit Test added; not run as requested. C++ format check passed. - Behavior changed: Yes, recycler now reclaims external versioned delete bitmap storage. - Does this need documentation: No
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
There was a problem hiding this comment.
Pull request overview
Fixes Apache Doris Cloud recycler’s handling of v2 delete-bitmap storage metadata by reading versioned delete-bitmap keys as blob values (which include a physical-key suffix due to blob_put), preventing false “missing delete bitmap” behavior during recycling.
Changes:
- Update
InstanceRecycler::decrement_delete_bitmap_packed_file_ref_countsto usecloud::blob_get+ValueBuf::to_pbinstead ofTransaction::get+ParseFromString. - Update recycler unit test helper to write v2 delete-bitmap storage metadata using
cloud::blob_putto match production behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| cloud/test/recycler_test.cpp | Aligns v2 delete-bitmap metadata setup with blob-based writes (blob_put) so tests exercise the real storage format. |
| cloud/src/recycler/recycler.cpp | Reads delete-bitmap storage metadata via blob APIs to correctly find suffixed physical keys and parse protobuf from merged blob fragments. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: the blob read fixes the production key-layout mismatch for the ordinary path-v0 case, but cleanup is still incomplete in three supported lifecycle/address variants. Each can report success and then discard the only DBM location while leaving a physical object or packed-file reference live; the inline comments identify clone ownership, legacy dropped-tablet cleanup, and path-version-1 standalone deletion.
Critical checkpoint conclusions:
- Goal and proof:
blob_getplusValueBuf::to_pbis the right logical read for a single legacy value or normal suffix/split blob, and the updated fixture matches the production blob encoding. It proves only standalone path-v0 cleanup; the three attached variants remain uncovered. - Scope and reuse: the code change is small and uses the existing blob abstraction. Transaction/read/parse failures still propagate with identifiers, and no new allocation, hot-loop, memory-safety, or performance issue was found.
- Concurrency and lifecycle: normal packed cleanup retains transaction-conflict retries, per-slice idempotency, and object-before-KV two-phase deletion. The blocking gaps occur where clone provenance, the legacy tablet path, or path reconstruction skips physical cleanup before metadata destruction.
- Compatibility and persistence: no new config, protocol, symbol, or storage format is introduced. Legacy unsuffixed and ordinary split values remain readable; existing clone ownership, multi-version mode, and storage-vault path-version fences expose the incomplete behavior.
- Testing and observability: tests cover the corrected blob encoding only for standalone path v0. They do not cover clone-last ownership, a packed DBM in legacy dropped-tablet recycling, or a path-v1 direct-write DBM. Existing logs also cannot reveal successful deletion of the wrong/missing object key or a corrected packed reference left live.
- User focus: no additional review focus was supplied, so the full PR and its parallel cleanup paths were reviewed.
No local build or test was run because the authoritative review-runner instructions prohibit builds. git diff --check is clean; the live style/license/title checks and macOS BE UT reported passing at submission time.
| std::string dbm_val; | ||
| err = txn->get(dbm_key, &dbm_val); | ||
| ValueBuf dbm_val; | ||
| err = cloud::blob_get(txn.get(), dbm_key, &dbm_val); |
There was a problem hiding this comment.
[P1] Read clone-shared DBM metadata from its owning instance. Classification and phase-3 cleanup deliberately key a clone rowset through RowsetMetaCloudPB::reference_instance_id, but this helper builds both the DBM key and packed-file key from the clone recycler's instance_id_. If the source recycler decrements the shared refcount first, the clone can be the last owner: this read returns NOT_FOUND, skips standalone/packed cleanup, and cleanup then removes the source DBM range, permanently losing the physical location. Thread the canonical/reference instance into this helper and use it for both keys; please cover clone-last standalone and packed DBMs.
| std::string dbm_val; | ||
| err = txn->get(dbm_key, &dbm_val); | ||
| ValueBuf dbm_val; | ||
| err = cloud::blob_get(txn.get(), dbm_key, &dbm_val); |
There was a problem hiding this comment.
[P1] Apply blob-backed DBM cleanup to the legacy dropped-tablet path too. When should_recycle_versioned_keys() is false, recycle_tablet() decrements only RowsetMetaCloudPB::packed_slice_locations, deletes data/<tablet>/, and then range-removes every v2 DBM KV. A packed DBM lives under data/packed_file and its exact slice location exists only in that DBM value, so a previously corrected packed record remains live permanently after the range removal. Process each visible/restore rowset's DBM before deleting that range, and add a non-versioned dropped-tablet case with a packed DBM.
| std::string dbm_val; | ||
| err = txn->get(dbm_key, &dbm_val); | ||
| ValueBuf dbm_val; | ||
| err = cloud::blob_get(txn.get(), dbm_key, &dbm_val); |
There was a problem hiding this comment.
[P1] Preserve the vault path format when deleting standalone DBMs. BE writes a path-version-1 HDFS-vault DBM as data/<shard>/<tablet>/<rowset>_delete_bitmap.db, but after this read returns STANDALONE_FILE both callers enqueue the fixed path-version-0 delete_bitmap_path. The accessors do not add the shard; deleting the nonexistent v0 object succeeds, and later metadata cleanup discards the only locator while the real v1 object remains. Retain the vault PathFormat or persist the exact standalone path, and cover a path-v1 direct-write DBM.
|
run buildall |
v2 delete bitmaps are written with
blob_put, which appends a suffix to every physical key.The recycler read the unsuffixed logical key with Transaction::get, treated existing delete bitmap metadata as missing, and skipped standalone-file deletion or packed-file reference-count updates.