github-actions[bot] commented on code in PR #64954:
URL: https://github.com/apache/doris/pull/64954#discussion_r3569815823
##########
be/src/storage/compaction/cumulative_compaction_policy.cpp:
##########
@@ -318,10 +318,6 @@ int
SizeBasedCumulativeCompactionPolicy::pick_input_rowsets(
DBUG_EXECUTE_IF("SizeBaseCumulativeCompactionPolicy.pick_input_rowsets.return_input_rowsets",
{ return transient_size; })
- if (total_size >= promotion_size) {
- return transient_size;
Review Comment:
This can still return empty for candidate sets that the score path says are
suitable. The guard checks `input_rowsets->size()`, but the vector is not
erased until after the loop, so in a two-rowset suffix it remains `2` after
`rs_begin` advances past the head. With a 1 GiB promotion size, a 900 MiB head
plus a 128 MiB/5-segment overlapping tail is schedulable because
`calc_cumulative_compaction_score()` still returns the aggregate score once
total size reaches promotion; this loop skips the head, then also evaluates the
final tail against remaining size 0 and erases everything, even though that
tail would pass the singleton-overlap checks below. The same regression hits
all-non-overlapping candidates whose aggregate size reaches promotion: before
this change they were compacted together and advanced the cumulative point, but
now they can be trimmed to empty and delayed until the long no-suitable
fallback. Please make the picker preserve aggregate-promotion progress and stop
the le
vel trim before deleting the final useful suffix rowset, with tests for both a
large head plus one overlapping tail and multiple non-overlapping rowsets that
are only promotion-sized in aggregate.
##########
be/src/cloud/cloud_cumulative_compaction_policy.cpp:
##########
@@ -172,10 +172,6 @@ int64_t
CloudSizeBasedCumulativeCompactionPolicy::pick_input_rowsets(
input_rowsets->push_back(rowset);
}
- if (total_size >= promotion_size) {
- return transient_size;
Review Comment:
Same problem here: after removing the pre-trim promotion-size return, this
loop can erase candidate sets that should still make progress.
`input_rowsets->size()` is the original vector size, so after a large head is
skipped the loop can still advance over the final rowset and `erase(begin,
end)`. That drops a large head plus one compactable overlapping tail, and it
also drops all-non-overlapping rowsets that are promotion-sized only in
aggregate, for example 900 MiB + 128 MiB with a 1 GiB `cloud_promotion_size()`.
Before this change those aggregate-promotion inputs were selected together and
the output could advance the cumulative point; now
`CloudCumulativeCompaction::pick_rowsets_to_compact()` returns
`CUMULATIVE_NO_SUITABLE_VERSION` and backs off. Please preserve
aggregate-promotion progress and stop the level trim before deleting the final
useful suffix rowset; add cloud tests for both the one-overlapping-tail case
and the multiple-non-overlapping-rowsets aggregate-promotion c
ase.
--
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]
