github-actions[bot] commented on code in PR #69523:
URL: https://github.com/apache/doris/pull/69523#discussion_r4358321177
##########
cloud/src/recycler/recycler.cpp:
##########
@@ -3010,14 +3010,16 @@ int InstanceRecycler::recycle_indexes() {
LOG_WARNING("malformed recycle index value").tag("key", hex(k));
return -1;
}
- if (index_pb.state() != RecycleIndexPB::RECYCLING) {
+ if (index_pb.state() == RecycleIndexPB::DROPPED ||
+ index_pb.state() == RecycleIndexPB::PREPARED) {
index_pb.set_state(RecycleIndexPB::RECYCLING);
txn->put(k, index_pb.SerializeAsString());
err = txn->commit();
if (err != TxnErrorCode::TXN_OK) {
LOG_WARNING("failed to commit txn").tag("err", err);
return -1;
}
+ return 0;
Review Comment:
[P2] Preserve dropped-index eligibility across the second pass
This return persists `RECYCLING` and advances the scanner past the index,
but `calculate_index_expired_time()` applies `dropped_index_retention_seconds`
only to `DROPPED`. With the default 3-hour dropped retention and 72-hour
general retention, an index marked after 3 hours is skipped on subsequent
passes until 72 hours; `TABLE_STREAM` takes the same branch. Its data and
metadata remain for roughly 69 extra hours. Treat `RECYCLING` records as
already eligible, or retain the selected deadline, so the deletion pass
preserves the configured dropped-index retention.
##########
cloud/src/meta-service/meta_service_partition.cpp:
##########
@@ -532,6 +534,14 @@ void
MetaServiceImpl::commit_index(::google::protobuf::RpcController* controller
LOG_INFO("remove recycle index").tag("key", hex(key));
txn->remove(key);
+ if (!is_versioned_read) {
+ // Keep the index visible even when it has no tablets or loses its
last partition.
+ auto placeholder_key = meta_tablet_key(
+ {instance_id, request->table_id(), index_id, INT64_MAX -
1, INT64_MAX - 1});
+ txn->put(placeholder_key, "");
Review Comment:
[P2] Gate placeholder writes until recycler readers are upgraded
This immediately writes a new persistent record for every legacy index
commit. Meta-service and recycler can run as separate processes, so a new
meta-service can share KV with an older recycler during an upgrade. The
pre-patch `recycle_tablets()` parses these empty bytes as `TabletMetaCloudPB`
with `tablet_id() == 0`; with the default
`enable_recycler_check_lazy_txn_finished=true`, it reads the nonexistent
tablet-index key for ID 0, fails, and leaves the placeholder and `RECYCLING`
index record on every retry. The new reader guards cannot protect that older
process. Gate marker emission until all consumers support it, or use an
existence record outside the legacy tablet-metadata family, and cover the
mixed-version path.
##########
cloud/src/recycler/recycler.cpp:
##########
@@ -3010,14 +3010,16 @@ int InstanceRecycler::recycle_indexes() {
LOG_WARNING("malformed recycle index value").tag("key", hex(k));
return -1;
}
- if (index_pb.state() != RecycleIndexPB::RECYCLING) {
+ if (index_pb.state() == RecycleIndexPB::DROPPED ||
+ index_pb.state() == RecycleIndexPB::PREPARED) {
index_pb.set_state(RecycleIndexPB::RECYCLING);
txn->put(k, index_pb.SerializeAsString());
err = txn->commit();
if (err != TxnErrorCode::TXN_OK) {
LOG_WARNING("failed to commit txn").tag("err", err);
return -1;
}
+ return 0;
Review Comment:
[P2] Update existing tests for the two-pass recycling contract
The new return also deterministically breaks tests independently of the
retention settings. `TableStreamRecyclerTest.RecycleStreamDeletesOnlyOffsets`
enables `force_immediate_recycle`, seeds a `DROPPED` stream, calls
`recycle_indexes()` once (table_stream_recycler_test.cpp:162), and expects the
recycle key and offsets gone, but this pass now only marks the key.
`RecyclerTest.recycle_indexes`/`recycle_versions` retain the same assumption,
while txn_lazy_commit_test.cpp:3266 now receives success before its expected
lazy-transaction failure. Assert the first marking pass and invoke the deletion
pass before the existing deletion/error assertions. The added placeholder test
calls `recycle_tablets()` directly and misses this transition.
--
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]
