github-actions[bot] commented on code in PR #63112:
URL: https://github.com/apache/doris/pull/63112#discussion_r3694981554
##########
be/src/cloud/cloud_cumulative_compaction_policy.cpp:
##########
@@ -159,7 +159,7 @@ int64_t
CloudSizeBasedCumulativeCompactionPolicy::pick_input_rowsets(
if (tablet->tablet_state() == TABLET_NOTREADY) {
// If tablet under alter, keep latest 10 version so that base
tablet max version
Review Comment:
[P1] Freeze the tablet state for the whole selection
With this new suffix filter, reading `tablet_state()` on every iteration can
produce a hole if schema change flips `NOTREADY` to `RUNNING` mid-loop: one of
the newest rowsets is skipped, then later rowsets are appended after the state
change. Neither path checks the filtered selection for an internal version
hole; compaction creates an output covering `[front.start, back.end]`, and the
cloud commit removes every rowset key in that range, including the skipped
rowset whose data was never read (the local path instead leaves overlapping
versions). The old `<` predicate only skipped a prefix, so this internal-hole
case is introduced by the direction change. Please take one state/max-version
snapshot under the header lock for the whole selection (or otherwise fence the
transition) and add a deterministic continuity test.
##########
be/src/storage/compaction/cumulative_compaction_policy.cpp:
##########
@@ -303,7 +303,7 @@ int SizeBasedCumulativeCompactionPolicy::pick_input_rowsets(
if (tablet->tablet_state() == TABLET_NOTREADY) {
// If tablet under alter, keep latest 10 version so that base
tablet max version
// not merged in new tablet, and then we can copy data from base
tablet
- if (rowset->version().second < max_version - 10) {
+ if (rowset->version().second > max_version - 10) {
continue;
Review Comment:
[P1] Keep the protected suffix out of the timeout fallback
When this filter leaves no input, the shared-nothing caller eventually
bypasses it: after `pick_rowset_to_compact_interval_sec` (86400 seconds by
default), any overlapping candidate makes
`CumulativeCompaction::pick_rowsets_to_compact()` assign the entire original
`candidate_rowsets` vector to `_input_rowsets`. A schema-change tablet that
remains `NOTREADY` for a day can therefore merge the newest ten versions that
this condition is meant to preserve. Please disable that fallback for
`NOTREADY` tablets or reapply the same immutable cutoff there, and cover the
aged-timestamp/overlapping-rowset path.
##########
be/src/cloud/cloud_cumulative_compaction_policy.cpp:
##########
@@ -159,7 +159,7 @@ int64_t
CloudSizeBasedCumulativeCompactionPolicy::pick_input_rowsets(
if (tablet->tablet_state() == TABLET_NOTREADY) {
// If tablet under alter, keep latest 10 version so that base
tablet max version
// not merged in new tablet, and then we can copy data from base
tablet
- if (rowset->version().second < max_version - 10) {
+ if (rowset->version().second > max_version - 10) {
continue;
Review Comment:
[P2] Base the cutoff on the maximum end version
`tablet->max_version()` returns the complete highest-ending rowset range,
but `max_version` above is initialized from `.first`. A `NOTREADY` tablet can
contain a multi-version maximum preserved by schema change or earlier
compaction; for candidates `[81]..[89], [90-100]`, this computes cutoff 80 and
skips everything, whereas the actual maximum 100 gives cutoff 90 and leaves
`[81]..[89]` eligible while protecting `[90-100]` whole. That stalls the
version-reduction behavior this code is meant to enable. Please snapshot/use
`.second` in both policies and cover a multi-version maximum.
--
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]
