morningman opened a new pull request, #69952:
URL: https://github.com/apache/doris/pull/69952
### What problem does this PR solve?
Issue Number: None
Related PR: #65810
Problem Summary:
**In short:** a changelog tablet that has nothing to compact, and on which
no compaction has succeeded for `pick_rowset_to_compact_interval_sec` (a day by
default), can abort the BE. Cumulative compaction then moves the cumulative
point to just after the start of the last candidate, and the candidates of a
changelog tablet start from version 0, so the new point can be smaller than the
current one and `Tablet::set_cumulative_layer_point` fails its CHECK. This PR
keeps changelog tablets out of that fallback; their own policy does not need it.
#### Background
- When `CumulativeCompaction::pick_rowsets_to_compact` picks nothing and
neither a base nor a cumulative compaction has succeeded on the tablet for
`pick_rowset_to_compact_interval_sec`, it falls back: if one candidate overlaps
it compacts every candidate at once, otherwise it moves the cumulative point to
`candidate_rowsets.back()->start_version() + 1`. This keeps base compaction,
which reads the rowsets below the point, going on a tablet that gets no new
rowsets.
- For an ordinary tablet the candidates start at the cumulative point, so
the new point is always above the old one.
- Since #65810 a changelog tablet compacts through the same
`CumulativeCompaction`, with `ChangelogCumulativeCompactionPolicy`. Its
candidates start from version 0
(`Tablet::pick_candidate_rowsets_to_changelog_compaction`), and its point means
something else: it ends the prefix of compact-enough LMax rowsets, which the
policy quick-merges instead of rewriting. The policy moves the point itself
(`calculate_cumulative_point`, `update_cumulative_point`), the tablet is not
scheduled for base compaction (`Tablet::can_do_compaction` allows only
`CHANGELOG_COMPACTION`), and the policy has a time trigger of its own
(`changelog_compaction_time_threshold_seconds` after the last successful
compaction). TTL cleanup is picked before the policy, in
`Compaction::pick_expired_changelog_rowset`.
#### The problem, and what it cost
- The point the fallback sets for a changelog tablet can be below the
current one, and `Tablet::set_cumulative_layer_point` CHECKs that it never goes
backwards, so the BE aborts. Two common layouts get there:
- a tablet compacted into the single LMax rowset `[0-5]` has its point at
6; a day later the fallback sets it to `0 + 1 = 1`;
- once `[0-5]` and the rowsets after it have expired, a TTL cleanup turns
them into one empty rowset `[0-N]` and leaves the point at 6
(`update_cumulative_point` skips an empty output); a day later, the same.
The first one survives a restart: the point is computed from the rowsets
again and the success times start over, so the BE aborts again a day later.
Seen on a local cluster, a day after the last compaction of the tablet: `Check
failed: new_point == Tablet::K_INVALID_CUMULATIVE_POINT || new_point >=
_cumulative_point Unexpected cumulative point: 1, origin: 2` in
`CumulativeCompaction::pick_rowsets_to_compact`.
- If one candidate overlaps, for example the output of a quick merge, the
fallback hands every candidate to one compaction: the whole tablet from version
0, rowsets of every level, past `changelog_level_compaction_max_deltas`.
- A day without compaction is the normal state of a changelog tablet that is
no longer written, e.g. in an old partition of a table partitioned by time.
#### How this PR fixes it
- In `CumulativeCompaction::pick_rowsets_to_compact` the fallback no longer
applies to a changelog tablet: it gets `CUMULATIVE_NO_SUITABLE_VERSION`, as
before the interval has passed, and keeps its point.
- The first-time initialization of the success times stays, since the time
trigger of the changelog policy reads the cumulative one.
- Ordinary tablets are unchanged. Cloud mode is not affected:
`CloudCumulativeCompaction` has no such fallback.
#### Results
| Changelog tablet with nothing to compact, no compaction for
`pick_rowset_to_compact_interval_sec` | Before this PR | After this PR |
| --- | --- | --- |
| One candidate overlaps (e.g. a quick-merge output) | Every candidate
compacted at once | Nothing compacted |
| No candidate overlaps, the last one starts two or more versions below the
point (e.g. only `[0-5]` left, point 6) | Point set back (to 1), BE aborts on
the CHECK | Point kept, nothing compacted |
| No candidate overlaps, otherwise | Point moved to just after the start of
the last candidate | Point kept, nothing compacted |
| A tablet without changelog | Fallback as above | Unchanged |
#### Classes, and how they call each other
- `CumulativeCompaction::pick_rowsets_to_compact`: the one changed condition.
-
`TestChangelogCumulativeCompactionPolicy.idle_tablet_keeps_cumulative_point`:
the new case.
```
CumulativeCompaction::prepare_compact
|-- Tablet::calculate_cumulative_point changelog policy: end of the
compact-enough LMax prefix
`-- CumulativeCompaction::pick_rowsets_to_compact
|-- Tablet::pick_candidate_rowsets_to_cumulative_compaction
| `-- changelog tablet:
pick_candidate_rowsets_to_changelog_compaction, from version 0
|-- Compaction::pick_expired_changelog_rowset TTL cleanup
|-- ChangelogCumulativeCompactionPolicy::pick_input_rowsets
`-- nothing picked, no compaction for
pick_rowset_to_compact_interval_sec
|-- changelog tablet (new): CUMULATIVE_NO_SUITABLE_VERSION,
point kept
`-- other tablets: compact every candidate if one overlaps,
else
Tablet::set_cumulative_layer_point(start of the last candidate + 1)
```
Not changed: the changelog policy, how its candidates are picked, and
`Tablet::set_cumulative_layer_point`.
### Release note
Fix a BE crash ("Unexpected cumulative point") on a tablet of a table with
changelog enabled that had nothing to compact for a day.
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [x] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
`TestChangelogCumulativeCompactionPolicy.idle_tablet_keeps_cumulative_point`: a
changelog tablet with only `[0-1]` left, the point at 2 and no compaction for
longer than `pick_rowset_to_compact_interval_sec`. `pick_rowsets_to_compact`
returns `CUMULATIVE_NO_SUITABLE_VERSION`, picks nothing and keeps the point,
for an overlapping and for a non-overlapping rowset. Without the fix the
overlapping case picks every candidate, and the non-overlapping one aborts with
`Unexpected cumulative point: 1, origin: 2`, the message seen on the cluster.
With the fix the 13 compaction and changelog suites around
`pick_rowsets_to_compact` pass, 141 cases:
`TestChangelogCumulativeCompactionPolicy`,
`TestSizeBasedCumulativeCompactionPolicy`,
`TestTimeSeriesCumulativeCompactionPolicy`, `CumulativeCompactionTest`,
`TestBaseCompaction`, `OrderedDataCompactionTest`, `VerticalCompactionTest`,
`CompactionTaskTest`, `CompactionScoreTest`, `CompactionMetricsTest`,
`ChangelogTtlTest`, `ChangelogVmergeCompactionTest` and
`RowTtlFullCompactionTest`. They ran on a local ASAN UT build (macOS) whose
storage, tablet and compaction sources are those of master; both changed files
also compile against master.
- Behavior changed:
- [ ] No.
- [x] Yes. <!-- Explain the behavior change -->
A changelog tablet that has had nothing to compact for
`pick_rowset_to_compact_interval_sec` keeps its cumulative point and starts no
compaction, instead of aborting the BE, moving its point or compacting all its
rowsets at once.
- Does this need documentation?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR should
merge into -->
--
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]
