github-actions[bot] commented on code in PR #65534:
URL: https://github.com/apache/doris/pull/65534#discussion_r3570141468
##########
cloud/src/recycler/recycler.cpp:
##########
@@ -3131,14 +3146,19 @@ int InstanceRecycler::delete_rowset_data(const
RowsetMetaCloudPB& rs_meta_pb) {
int64_t tablet_id = rs_meta_pb.tablet_id();
const auto& rowset_id = rs_meta_pb.rowset_id_v2();
for (int64_t i = 0; i < num_segments; ++i) {
- file_paths.push_back(segment_path(tablet_id, rowset_id, i));
+ add_file_to_delete_if_not_packed(rs_meta_pb, segment_path(tablet_id,
rowset_id, i),
Review Comment:
This packed-slice guard is not reached for `BEGIN_PARTIAL_UPDATE` rowsets.
`CloudRowsetWriter::build()` collects `packed_slice_locations` before it
switches partial-update rowsets to `BEGIN_PARTIAL_UPDATE`, but both recycler
paths still route that state to `delete_rowset_data(resource_id, tablet_id,
rowset_id)` / `delete_prefix(rowset_path_prefix(...))` before entering this
loop. That means an aborted partial-update tmp rowset with packed slices still
deletes every `data/{tablet}/{rowset_id}_*` object by prefix, bypassing the
protection this patch adds for the normal explicit-file path. Please make the
prefix cleanup packed-aware as well, for example by listing the prefix and
excluding paths present in `packed_slice_locations`, and add coverage for
packed `BEGIN_PARTIAL_UPDATE` tmp rowsets.
##########
cloud/src/recycler/recycler.cpp:
##########
@@ -3882,11 +3902,15 @@ int InstanceRecycler::delete_rowset_data(
continue;
}
for (int64_t i = 0; i < num_segments; ++i) {
- file_paths.push_back(segment_path(tablet_id, rowset_id, i));
+ add_file_to_delete_if_not_packed(rs, segment_path(tablet_id,
rowset_id, i),
Review Comment:
In the all-packed case this helper can skip every segment/index path for a
rowset, leaving `file_paths` empty. The batch metrics below are updated only by
iterating the paths passed to `delete_files()` and parsing a rowset id from
each path, while empty deletes return success. So a rowset can have its packed
ref counts decremented and its recycle/tmp KVs removed, but
`total_recycled_num`, `total_recycled_data_size`, and
`segment_metrics_context_` never include it. Please account successful rowsets
independently of the non-empty object path list, and add a regression to the
new packed batch test that checks the recycler metrics.
--
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]
