github-actions[bot] commented on code in PR #67272:
URL: https://github.com/apache/doris/pull/67272#discussion_r3910124991
##########
cloud/src/recycler/recycler.cpp:
##########
@@ -4524,6 +4521,20 @@ int InstanceRecycler::delete_rowset_data(
ret = -1;
continue;
}
+
+ // we have to treat tmp rowset as "orphans" that may not related to
any existing tablets
Review Comment:
[P1] Match the packed bitmap slice by its stored location
Moving the recycled-tablet check below the two decrement calls makes
retained recycle rowsets reliably depend on the existing bitmap helper. A
resource-wide packed file can contain segment/index slices and the
delete-bitmap slice for the same tablet/rowset, and slice order comes from an
`unordered_map`. After `decrement_packed_file_ref_counts()` marks a segment
deleted, the bitmap helper matches only tablet/rowset and stops at that first
deleted segment, treating the bitmap as already deleted while its actual
slice/refcount remains live. Once the bitmap blob and recycle key are removed,
an already-corrected packed record is never rescanned, so the object/KV leaks
permanently. Please match the bitmap's persisted packed path/offset/size and
add a colocated-slices test.
##########
cloud/src/recycler/recycler.cpp:
##########
@@ -4524,6 +4521,20 @@ int InstanceRecycler::delete_rowset_data(
ret = -1;
continue;
}
+
+ // we have to treat tmp rowset as "orphans" that may not related to
any existing tablets
+ // due to aborted schema change.
+ if (is_formal_rowset) {
+ std::lock_guard lock(recycled_tablets_mtx_);
+ if (recycled_tablets_.contains(rs.tablet_id()) &&
Review Comment:
[P1] Do not skip recycle rowsets from an unswept vault
`recycled_tablets_` only means `recycle_tablet()` deleted the tablet prefix
from resource IDs found in current formal/restore rowsets. In a cross-vault
tablet, a compacted recycle rowset can still point to old vault A while
surviving formal rowsets point only to vault B; tablet recycling sweeps B, this
branch then skips deletion from A, and the caller removes the recycle key,
leaking A's segment/index and standalone-bitmap files permanently. Please
include retained recycle-rowset vaults in the tablet sweep, scope the marker to
`(tablet_id, resource_id)`, or perform the idempotent rowset deletion here.
##########
cloud/src/recycler/recycler.cpp:
##########
@@ -5315,11 +5326,17 @@ int InstanceRecycler::recycle_tablet(int64_t tablet_id,
RecyclerMetricsContext&
std::string delete_bitmap_end = meta_delete_bitmap_key({instance_id_,
tablet_id + 1, "", 0, 0});
txn->remove(delete_bitmap_start, delete_bitmap_end);
- std::string dbm_start_key =
versioned::meta_delete_bitmap_key({instance_id_, tablet_id, ""});
- std::string dbm_end_key = versioned::meta_delete_bitmap_key({instance_id_,
tablet_id + 1, ""});
- txn->remove(dbm_start_key, dbm_end_key);
- LOG(INFO) << "remove delete bitmap kv, tablet=" << tablet_id << ", begin="
<< hex(dbm_start_key)
- << " end=" << hex(dbm_end_key);
+ for (const auto& rowset_id : versioned_delete_bitmap_rowset_ids) {
Review Comment:
[P1] Batch the per-rowset bitmap range clears
Each iteration adds a separate clear range, including two encoded keys plus
FDB mutation/conflict-range overhead, to the same final transaction.
Time-series tablets can reach the default 20,000-version limit; with normal
48-byte rowset IDs these clears alone can exceed Doris's 7-8 MiB safe
transaction envelope and commit with `TXN_BYTES_TOO_LARGE`. Because the rowset
ranges are removed in that same failed transaction, every retry reconstructs
the identical oversized transaction and the dropped index/partition cannot
finish recycling. Please commit these idempotent bitmap clears in bounded
batches using `approximate_bytes()`/`max_txn_commit_byte`, then perform the
final tablet metadata cleanup.
##########
cloud/test/recycler_test.cpp:
##########
@@ -3686,8 +3696,216 @@ TEST(RecyclerTest,
recycle_tablet_packed_file_ref_count) {
ASSERT_FALSE(list_iter->has_next());
}
+TEST(RecyclerTest, recycle_tablet_recycle_rowset_packed_file_ref_count) {
+ const bool old_force_immediate_recycle = config::force_immediate_recycle;
+ DORIS_CLOUD_DEFER {
+ config::force_immediate_recycle = old_force_immediate_recycle;
+ };
+ config::force_immediate_recycle = true;
+ auto txn_kv = std::make_shared<MemTxnKv>();
+ ASSERT_EQ(txn_kv->init(), 0);
+
+ constexpr std::string_view kResourceId =
"recycle_tablet_recycle_rowset_ref";
+ auto instance = create_recycler_test_instance(std::string(kResourceId));
+ InstanceRecycler recycler(txn_kv, instance, thread_group,
+ std::make_shared<TxnLazyCommitter>(txn_kv));
+ ASSERT_EQ(recycler.init(), 0);
+ auto accessor = recycler.accessor_map_.begin()->second;
+
+ constexpr int64_t tablet_id = 40101;
+ constexpr int64_t index_id = 40102;
+ constexpr int64_t kSmallFileSize = 64;
+ doris::TabletSchemaCloudPB schema;
+ schema.set_schema_version(1);
+
+ auto formal_rowset = create_rowset(std::string(kResourceId), tablet_id,
index_id, 1, schema,
+ RowsetStatePB::VISIBLE);
+ formal_rowset.set_end_version(1);
+
+ auto recycled_rowset = create_rowset(std::string(kResourceId), tablet_id,
index_id, 1, schema,
+ RowsetStatePB::VISIBLE);
+ const std::string small_file_path = segment_path(tablet_id,
recycled_rowset.rowset_id_v2(), 0);
+ const std::string packed_file_path =
"data/packed_file/1/recycle_rowset.bin";
+ auto& packed_location =
(*recycled_rowset.mutable_packed_slice_locations())[small_file_path];
+ packed_location.set_packed_file_path(packed_file_path);
+ packed_location.set_offset(0);
+ packed_location.set_size(kSmallFileSize);
+
+ RecycleRowsetPB recycle_rowset;
+ recycle_rowset.set_creation_time(current_time);
+ recycle_rowset.set_type(RecycleRowsetPB::COMPACT);
+ recycle_rowset.mutable_rowset_meta()->CopyFrom(recycled_rowset);
+ const std::string recycled_rowset_key =
+ recycle_rowset_key({instance_id, tablet_id,
recycled_rowset.rowset_id_v2()});
+
+ // A corrected packed file will not have its rowset references scanned
again.
+ PackedFileInfoPB packed_info;
+ packed_info.set_ref_cnt(1);
+ packed_info.set_total_slice_num(1);
+ packed_info.set_total_slice_bytes(kSmallFileSize);
+ packed_info.set_remaining_slice_bytes(kSmallFileSize);
+ packed_info.set_corrected(true);
+ packed_info.set_state(PackedFileInfoPB::NORMAL);
+ packed_info.set_resource_id(std::string(kResourceId));
+ auto* packed_slice = packed_info.add_slices();
+ packed_slice->set_path(small_file_path);
+ packed_slice->set_offset(0);
+ packed_slice->set_size(kSmallFileSize);
+ packed_slice->set_deleted(false);
+ packed_slice->set_corrected(true);
+ packed_slice->set_tablet_id(tablet_id);
+ packed_slice->set_rowset_id(recycled_rowset.rowset_id_v2());
+ packed_slice->set_txn_id(next_small_file_txn_id());
+ const std::string packed_key = packed_file_key({instance_id,
packed_file_path});
+
+ std::unique_ptr<Transaction> txn;
+ ASSERT_EQ(TxnErrorCode::TXN_OK, txn_kv->create_txn(&txn));
+ txn->put(meta_rowset_key({instance_id, tablet_id,
formal_rowset.end_version()}),
+ formal_rowset.SerializeAsString());
+ txn->put(recycled_rowset_key, recycle_rowset.SerializeAsString());
+ txn->put(packed_key, packed_info.SerializeAsString());
+ ASSERT_EQ(TxnErrorCode::TXN_OK, txn->commit());
+
+ ASSERT_EQ(0, accessor->put_file(packed_file_path, "payload"));
+ const std::string formal_segment_path =
+ segment_path(tablet_id, formal_rowset.rowset_id_v2(), 0);
+ ASSERT_EQ(0, accessor->put_file(formal_segment_path, "segment"));
+
+ ASSERT_EQ(0, recycler.recycle_tablet(tablet_id, ctx));
+ ASSERT_EQ(0, recycler.recycle_rowsets());
+ ASSERT_EQ(0, recycler.recycle_packed_files());
+
+ ASSERT_EQ(TxnErrorCode::TXN_OK, txn_kv->create_txn(&txn));
+ std::string value;
+ EXPECT_EQ(TxnErrorCode::TXN_KEY_NOT_FOUND, txn->get(recycled_rowset_key,
&value));
+ EXPECT_EQ(TxnErrorCode::TXN_KEY_NOT_FOUND, txn->get(packed_key, &value));
+ EXPECT_EQ(1, accessor->exists(packed_file_path));
+}
+
+TEST(RecyclerTest,
recycle_tablet_recycle_rowset_delete_bitmap_packed_file_ref_count) {
+ const bool old_force_immediate_recycle = config::force_immediate_recycle;
+ DORIS_CLOUD_DEFER {
+ config::force_immediate_recycle = old_force_immediate_recycle;
+ };
+ config::force_immediate_recycle = true;
+
+ auto txn_kv = std::make_shared<MemTxnKv>();
+ ASSERT_EQ(txn_kv->init(), 0);
+
+ constexpr std::string_view kResourceId =
"recycle_tablet_recycle_rowset_dbm_ref";
+ auto instance = create_recycler_test_instance(std::string(kResourceId));
+ InstanceRecycler recycler(txn_kv, instance, thread_group,
+ std::make_shared<TxnLazyCommitter>(txn_kv));
+ ASSERT_EQ(recycler.init(), 0);
+ auto accessor = recycler.accessor_map_.begin()->second;
+
+ constexpr int64_t tablet_id = 40103;
+ constexpr int64_t index_id = 40104;
+ constexpr int64_t kSmallFileSize = 64;
+ doris::TabletSchemaCloudPB schema;
+ schema.set_schema_version(1);
+
+ auto formal_rowset = create_rowset(std::string(kResourceId), tablet_id,
index_id, 1, schema,
+ RowsetStatePB::VISIBLE);
+ formal_rowset.set_end_version(1);
+
+ auto restore_job_rowset = create_rowset(std::string(kResourceId),
tablet_id, index_id, 1,
+ schema, RowsetStatePB::VISIBLE);
+ restore_job_rowset.set_end_version(2);
+ const std::string restore_job_rowset_key =
+ job_restore_rowset_key({instance_id, tablet_id,
restore_job_rowset.end_version()});
+
+ auto recycled_rowset = create_rowset(std::string(kResourceId), tablet_id,
index_id, 1, schema,
+ RowsetStatePB::VISIBLE);
+ RecycleRowsetPB recycle_rowset;
+ recycle_rowset.set_creation_time(current_time);
+ recycle_rowset.set_type(RecycleRowsetPB::COMPACT);
+ recycle_rowset.mutable_rowset_meta()->CopyFrom(recycled_rowset);
+ const std::string recycled_rowset_key =
+ recycle_rowset_key({instance_id, tablet_id,
recycled_rowset.rowset_id_v2()});
+
+ const std::string packed_file_path =
"data/packed_file/1/recycle_rowset_delete_bitmap.bin";
+ const std::string delete_bitmap_key = versioned::meta_delete_bitmap_key(
+ {instance_id, tablet_id, recycled_rowset.rowset_id_v2()});
+ const std::string formal_delete_bitmap_key =
versioned::meta_delete_bitmap_key(
+ {instance_id, tablet_id, formal_rowset.rowset_id_v2()});
+ const std::string restore_job_delete_bitmap_key =
versioned::meta_delete_bitmap_key(
+ {instance_id, tablet_id, restore_job_rowset.rowset_id_v2()});
+ DeleteBitmapStoragePB delete_bitmap_storage;
+ delete_bitmap_storage.set_store_in_fdb(false);
+ auto* packed_location =
delete_bitmap_storage.mutable_packed_slice_location();
+ packed_location->set_packed_file_path(packed_file_path);
+ packed_location->set_offset(0);
+ packed_location->set_size(kSmallFileSize);
+
+ PackedFileInfoPB packed_info;
+ packed_info.set_ref_cnt(1);
+ packed_info.set_total_slice_num(1);
+ packed_info.set_total_slice_bytes(kSmallFileSize);
+ packed_info.set_remaining_slice_bytes(kSmallFileSize);
+ packed_info.set_corrected(true);
+ packed_info.set_state(PackedFileInfoPB::NORMAL);
+ packed_info.set_resource_id(std::string(kResourceId));
+ auto* packed_slice = packed_info.add_slices();
+ packed_slice->set_path(delete_bitmap_path(tablet_id,
recycled_rowset.rowset_id_v2()));
+ packed_slice->set_offset(0);
+ packed_slice->set_size(kSmallFileSize);
+ packed_slice->set_deleted(false);
+ packed_slice->set_corrected(true);
+ packed_slice->set_tablet_id(tablet_id);
+ packed_slice->set_rowset_id(recycled_rowset.rowset_id_v2());
+ packed_slice->set_txn_id(next_small_file_txn_id());
+ const std::string packed_key = packed_file_key({instance_id,
packed_file_path});
+
+ std::unique_ptr<Transaction> txn;
+ ASSERT_EQ(TxnErrorCode::TXN_OK, txn_kv->create_txn(&txn));
+ txn->put(meta_rowset_key({instance_id, tablet_id,
formal_rowset.end_version()}),
+ formal_rowset.SerializeAsString());
+ txn->put(restore_job_rowset_key, restore_job_rowset.SerializeAsString());
+ txn->put(recycled_rowset_key, recycle_rowset.SerializeAsString());
+ DeleteBitmapStoragePB in_fdb_delete_bitmap_storage;
+ in_fdb_delete_bitmap_storage.set_store_in_fdb(true);
+ cloud::blob_put(txn.get(), formal_delete_bitmap_key,
in_fdb_delete_bitmap_storage, 0);
+ cloud::blob_put(txn.get(), restore_job_delete_bitmap_key,
in_fdb_delete_bitmap_storage, 0);
+ cloud::blob_put(txn.get(), delete_bitmap_key, delete_bitmap_storage, 0);
+ txn->put(packed_key, packed_info.SerializeAsString());
+ ASSERT_EQ(TxnErrorCode::TXN_OK, txn->commit());
+
+ ASSERT_EQ(0, accessor->put_file(packed_file_path, "delete bitmap
payload"));
+ const std::string formal_segment_path =
+ segment_path(tablet_id, formal_rowset.rowset_id_v2(), 0);
+ ASSERT_EQ(0, accessor->put_file(formal_segment_path, "segment"));
+
+ ASSERT_EQ(0, recycler.recycle_tablet(tablet_id, ctx));
+
+ ASSERT_EQ(TxnErrorCode::TXN_OK, txn_kv->create_txn(&txn));
+ ValueBuf delete_bitmap_value;
+ EXPECT_EQ(TxnErrorCode::TXN_OK,
+ cloud::blob_get(txn.get(), delete_bitmap_key,
&delete_bitmap_value));
+ ValueBuf removed_delete_bitmap_value;
+ EXPECT_EQ(TxnErrorCode::TXN_KEY_NOT_FOUND,
+ cloud::blob_get(txn.get(), formal_delete_bitmap_key,
&removed_delete_bitmap_value));
+ EXPECT_EQ(TxnErrorCode::TXN_KEY_NOT_FOUND,
+ cloud::blob_get(txn.get(), restore_job_delete_bitmap_key,
+ &removed_delete_bitmap_value));
+
+ ASSERT_EQ(0, recycler.recycle_rowsets());
+ ASSERT_EQ(0, recycler.recycle_packed_files());
+
+ ASSERT_EQ(TxnErrorCode::TXN_OK, txn_kv->create_txn(&txn));
+ std::string value;
+ EXPECT_EQ(TxnErrorCode::TXN_KEY_NOT_FOUND, txn->get(recycled_rowset_key,
&value));
+ EXPECT_EQ(TxnErrorCode::TXN_KEY_NOT_FOUND, txn->get(packed_key, &value));
+ EXPECT_EQ(TxnErrorCode::TXN_KEY_NOT_FOUND, txn->get(delete_bitmap_key,
&value));
Review Comment:
[P2] Check the blob chunks instead of the unwritten prefix
`cloud::blob_put` never writes `delete_bitmap_key` itself; it appends an
encoded chunk suffix to every physical KV. Consequently this exact
`txn->get(delete_bitmap_key, ...)` returns `TXN_KEY_NOT_FOUND` even immediately
after setup, so the test still passes if deferred recycling leaves every
delete-bitmap chunk behind.
```suggestion
EXPECT_EQ(TxnErrorCode::TXN_KEY_NOT_FOUND,
cloud::blob_get(txn.get(), delete_bitmap_key,
&removed_delete_bitmap_value));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
