morningman opened a new pull request, #69240:
URL: https://github.com/apache/doris/pull/69240
### What problem does this PR solve?
Issue Number: None
Related PR: #68033, #65810
Problem Summary:
**In short.** In local (non-cloud) mode, altering a compaction setting of a
table with `binlog.format = 'ROW'` also reaches its hidden row binlog tablets.
A new `compaction_policy` moves them off the `binlog` policy they must use. A
`time_series_compaction_*` change on a `time_series` table fails halfway.
#68033 fixed the same thing for cloud. This PR applies the same fix to the
local path: the FE no longer sends compaction settings to row binlog tablets.
As a second guard, the BE now refuses to change a row binlog tablet's policy.
**Background.**
- A table with `binlog.format = 'ROW'` has a hidden row binlog index. Each
data tablet has a row binlog tablet next to it, which stores the change events.
- Row binlog tablets are created with the `binlog` compaction policy
(`BinlogCumulativeCompactionPolicy`). It is a tiered policy written for change
events; for example, its quick merge links the segments of several rowsets into
one `OVERLAPPING` rowset instead of rewriting them. `CreateReplicaTask` sets it
when it creates a row binlog tablet.
- `ALTER TABLE ... SET (<compaction property>)` sends the new value to the
tablets through `UpdateTabletMetaInfoTask`. #68033 made the cloud handler
(`CloudSchemaChangeHandler`) leave out row binlog tablets for
`compaction_policy`. The local handler
(`SchemaChangeHandler.updatePartitionProperties`) still uses
`getMaterializedIndices(VISIBLE, true)`, so the row binlog index is included.
**The problem, and what it cost.** Both cases are reproduced on master in
local mode:
| Statement on a table with `binlog.format = 'ROW'` | What happens on master
|
|---|---|
| `ALTER TABLE t SET ("compaction_policy" = "time_series")` | Succeeds, and
every row binlog tablet meta now says `time_series`. From the next compaction
on, those tablets are compacted by the time series policy instead of the binlog
policy. |
| `ALTER TABLE t SET ("time_series_compaction_goal_size_mbytes" = "2048")`
on a `time_series` table | Fails: `Failed to update partition[t]. tablet meta.
Error: ...[INVALID_ARGUMENT]only time series compaction policy support time
series config]. This operation maybe partial successfully, You should retry
until success.` The BE rejects time series settings for tablets that are not on
the time series policy, and the row binlog tablets are on `binlog`. The data
tablets are already updated by then. Retrying fails the same way. |
**How this PR fixes it.**
1. FE, `SchemaChangeHandler.updatePartitionProperties`: when the update
carries a compaction policy or a time series setting, the row binlog index is
left out. This is the same rule #68033 uses for cloud.
- `ALTER TABLE ... SET` changes one table property at a time
(`ModifyTablePropertiesOp.validate`), so such an update has nothing else for
the row binlog tablets.
- Every other tablet meta update (in memory, storage policy, binlog
config, `disable_auto_compaction`, ...) still reaches them, as before.
2. BE, `update_tablet_meta_callback`:
- It refuses to change the compaction policy of a row binlog tablet (`can
not change compaction policy of row binlog tablet <id>`).
- It no longer accepts `binlog` as a new policy for any tablet. #65810
added it to the accepted values, but no FE path sends it: the policy is only
set when a row binlog tablet is created.
- With these checks, an older FE, or a future caller, gets an error
instead of silently moving a row binlog tablet off its policy.
**Results.**
| Statement (local mode, table with `binlog.format = 'ROW'`) | Before |
After |
|---|---|---|
| `SET ("compaction_policy" = "time_series")` | Succeeds; row binlog tablets
switch to `time_series` | Succeeds; data tablets switch, row binlog tablets
stay on `binlog` |
| `SET ("time_series_compaction_goal_size_mbytes" = "2048")` on a
`time_series` table | Fails after updating the data tablets | Succeeds; data
tablets get 2048, row binlog tablets are untouched |
| The first statement from an FE without this fix, against a BE with it |
Succeeds; row binlog tablets switch to `time_series` | Fails with `can not
change compaction policy of row binlog tablet <id>`; row binlog tablets stay on
`binlog` |
### Release note
Altering `compaction_policy` or a `time_series_compaction_*` property of a
table with `binlog.format = 'ROW'` no longer changes the compaction policy of
its hidden row binlog tablets, and no longer fails for the time series
properties.
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [x] Regression test
- [x] Unit Test
- [x] 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? -->
- Regression test
`row_binlog_p0/test_row_binlog_alter_compaction_policy` (local mode only, like
`test_binlog_compaction`). It alters the policy of one table and a time series
setting of another. After a manual cumulative compaction it checks each
tablet's policy, both in the tablet meta (`/api/meta/header`) and in the policy
the tablet compacts with (`/api/compaction/show`). It also reads the change
events back through `binlog()`. Before the fix it fails with `expected:
<binlog> but was: <time_series>`; with the fix it passes. All 24 suites of
`row_binlog_p0` pass with the fix.
- FE unit test
`SchemaChangeHandlerTest#testAlterCompactionSettingSkipsRowBinlogTablets`. It
records the tablets of every `UpdateTabletMetaInfoTask` the ALTER builds:
- `compaction_policy` and a time series setting reach only the data
tablets;
- `disable_auto_compaction` still reaches the row binlog tablets.
With the FE change reverted, the test fails. The whole
`SchemaChangeHandlerTest` (37 tests) passes.
- Build: `build.sh --fe --be` (BE Release; FE with checkstyle).
- Manual test: a BE with this fix and an FE without it. `SET
("compaction_policy" = "time_series")` fails with `can not change compaction
policy of row binlog tablet <id>`, and that tablet's meta still says `binlog`.
- Cloud mode is not touched: #68033 already covers the policy, with
`CloudSchemaChangeHandlerTest`. In cloud mode the time series settings still
reach row binlog tablets. Nothing fails there, because the meta service stores
them without the policy check and the binlog policy never reads them.
- Behavior changed:
- [ ] No.
- [x] Yes. See the Release note. In addition, the BE rejects a
compaction policy change on a row binlog tablet, and `binlog` as a new
compaction policy.
- 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]
