[fix](compaction) Keep latest 10 versions on new tablet during picking compaction input rowsets when schema change - #63112
Closed
Hastyshell wants to merge 1 commit into
Conversation
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: The NOTREADY tablet cumulative compaction policy intended to keep the latest 10 versions from being compacted during alter, but the condition skipped older versions instead. This change makes the condition match the documented behavior. ### Release note None ### Check List (For Author) - Test: No need to test (condition-only fix; verified with git diff --check) - Behavior changed: Yes (NOTREADY tablet compaction now preserves the latest versions as documented) - Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Collaborator
Author
|
run buildall |
Hastyshell
changed the title
Hastyshell
changed the title
luwei16
approved these changes
May 9, 2026
Contributor
|
PR approved by at least one committer and no changes requested. |
github-actions
Bot
added
the
approved
Contributor
|
PR approved by anyone and no changes requested. |
Contributor
TPC-H: Total hot run time: 29557 ms |
Contributor
TPC-DS: Total hot run time: 172523 ms |
Contributor
TPC-H: Total hot run time: 29636 ms |
Contributor
TPC-DS: Total hot run time: 172039 ms |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
12 tasks
Contributor
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Request changes. The predicate direction is correct for a stable singleton maximum, but two reachable paths still bypass the intended NOTREADY protection and a third miscomputes the boundary for a multi-version maximum.
Critical checkpoint conclusions:
- Goal and proof: The change does not yet correctly enforce the ten-version boundary: a state transition can create a non-contiguous selection, the local timeout path can restore the protected candidates, and a multi-version maximum moves the cutoff backward and can stall eligible version reduction. No direct
TABLET_NOTREADYpolicy test proves the behavior; live incremental coverage for this head is 50% (1/2 changed lines). - Scope and focus: The implementation change is otherwise small, clear, and mirrored across cloud and shared-nothing size-based policies. The time-series policy intentionally rejects
NOTREADY; the local caller's timeout fallback is the material parallel path the patch misses. No extra review focus was supplied, so the whole PR was reviewed. - Concurrency and lifecycle: Non-MoW schema-change completion can flip the plain tablet state after candidate locking and during per-row selection. There is no new lock order or deadlock issue, but the state/max snapshot is not stable for the selection lifecycle.
- Configuration and compatibility: No configuration, dynamic-update, ABI/symbol, storage-format, edit-log, FE-BE variable, protocol, or rolling-upgrade change is introduced. The pre-existing 86400-second timeout configuration exposes one accepted bypass.
- Conditions and parallel paths: The corrected
>condition is appropriate only with one stable state and the actual maximum end version. Cloud and local callers, delete handling, threshold-crossing rowsets, full-compaction routes, and time-series selection were checked; the separate BE-admin full-compaction behavior predates and does not execute these changed policies. - Transactions and data correctness: A mid-loop hole can make cloud commit delete a skipped rowset inside the advertised output range; the local path can install an output overlapping the skipped version. The timeout path can merge the suffix that schema change needs kept separate. These are blocking correctness issues.
- Memory, errors, performance, and observability: The patch adds no allocation, ownership, memory-tracking, error-propagation, or meaningful complexity concern. Existing logs do not replace the missing immutable snapshot and post-filter continuity guarantee; no new metric is necessary for this narrow fix.
- Tests and results: The authoritative change adds no tests or result files. Existing green BE/Cloud unit-test jobs provide no direct
TABLET_NOTREADYpolicy test proving the boundary semantics, and the coverage check reports only 1/2 changed lines covered. Per the supplied review contract this was a static-only review; no build or test command was run locally. - Review completion: All code candidates were independently verified and adjudicated across the maximum three rounds. The final risk pass found only the wording corrections incorporated above; because the corrected bytes cannot receive a fourth convergence round, this review is incomplete under the prompt's strict convergence definition even though every substantiated code issue is included below.
| @@ -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 | |||
Contributor
There was a problem hiding this 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.
| // 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; |
Contributor
There was a problem hiding this 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.
| // 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; |
Contributor
There was a problem hiding this 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.
gavinchou
pushed a commit
that referenced
this pull request
Aug 6, 2026
…ge (#66337) Related PR: #63112 Problem Summary: This PR takes over #63112 and adds explicit local and Cloud unit-test coverage. While a schema change target tablet is `TABLET_NOTREADY`, cumulative compaction should merge only older rowsets and leave the newest 10 versions unmerged. The filter used the inverse comparison, skipping older rowsets and selecting the newest versions. A compaction output could then cross the base tablet's maximum version and prevent incremental schema-change conversion with `VERSION_ALREADY_MERGED`. This change reverses the comparison in both local and Cloud size-based cumulative compaction policies. The new tests verify that versions 2 through 10 are selected and versions 11 through 20 remain unmerged. ### Release note Fix schema changes that could fail when cumulative compaction merged the latest versions on the new tablet.
Collaborator
wyxxxcat
pushed a commit
to wyxxxcat/doris
that referenced
this pull request
Aug 17, 2026
…ge (apache#66337) Related PR: apache#63112 Problem Summary: This PR takes over apache#63112 and adds explicit local and Cloud unit-test coverage. While a schema change target tablet is `TABLET_NOTREADY`, cumulative compaction should merge only older rowsets and leave the newest 10 versions unmerged. The filter used the inverse comparison, skipping older rowsets and selecting the newest versions. A compaction output could then cross the base tablet's maximum version and prevent incremental schema-change conversion with `VERSION_ALREADY_MERGED`. This change reverses the comparison in both local and Cloud size-based cumulative compaction policies. The new tests verify that versions 2 through 10 are selected and versions 11 through 20 remain unmerged. ### Release note Fix schema changes that could fail when cumulative compaction merged the latest versions on the new tablet.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: The cumulative compaction policy for NOTREADY tablets is intended to keep the latest 10 versions from being compacted during alter, but the condition skipped older versions instead. This change makes the condition match the documented behavior.
Release note
None
Check List (For Author)
git diff --check)