morningman opened a new pull request, #68982:
URL: https://github.com/apache/doris/pull/68982
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
#### In short
The table and database property `binlog.format` only accepted the exact
upper case names `ROW` and `STATEMENT_AND_SNAPSHOT`. `'binlog.format' = 'row'`
was rejected by every DDL, and CREATE TABLE even answered with Java's internal
`No enum constant ...` message. This PR matches the value case-insensitively at
every place it is parsed and always stores the canonical upper case name,
including for database properties, which Doris persists as written and parses
again on replay. Unknown values are still rejected, now with the same message
everywhere.
#### Background
- `binlog.format` is a table and database property. `ROW` turns on row
binlog (table streams, IVM, the `binlog()` TVF); `STATEMENT_AND_SNAPSHOT` is
the CCR binlog. A table's format cannot be changed after it is created.
- The value is parsed in three places, all with `BinlogFormat.valueOf`,
which is case-sensitive:
- `PropertyAnalyzer#analyzeBinlogConfig`: validation used by ALTER TABLE,
CREATE DATABASE and ALTER DATABASE (and by CREATE TABLE, but only later, see
below).
- `BinlogConfig#mergeFromProperties`: builds an effective config. CREATE
TABLE calls it on the raw properties first, in
`Database#getBinlogConfigsForCreateTable`, before anything validates them.
Database properties also go through it on every replay and image load
(`Database#gsonPostProcess`, `replayUpdateDbProperties`).
- `TableProperty#buildBinlogConfig`: rebuilds a table's config from its
persisted properties.
- Table metadata stores the format from the enum
(`BinlogConfig#toProperties`), so it is always canonical. Database properties
are different: `DatabaseProperty` keeps the map exactly as the user wrote it,
and that map is what goes to the edit log and the image.
- Neighbouring enum-like properties such as `storage_medium` and
`compression` are already matched case-insensitively.
#### The problem, and what it cost
- `'binlog.format' = 'row'`, or any spelling other than all upper case, is
rejected by all four DDLs that take the property. CREATE TABLE fails with `No
enum constant org.apache.doris.catalog.BinlogConfig.BinlogFormat.row`, which
does not even name the property; ALTER TABLE, CREATE DATABASE and ALTER
DATABASE fail with `Invalid binlog format value: row`.
- Users have to know that this one value is case-sensitive while the
properties next to it are not. The full before / after comparison is in Results
below.
#### How this PR fixes it
1. `BinlogConfig.BinlogFormat#fromString` looks the value up by enum name,
ignoring case, and throws `IllegalArgumentException("Invalid binlog format
value: <value>")` for anything else. The three parse sites above all use it.
2. `PropertyAnalyzer#analyzeBinlogConfig` returns the canonical name, so the
table property, `SHOW CREATE TABLE` and `information_schema.table_properties`
show `ROW`.
3. `PropertyAnalyzer#normalizeBinlogFormat`, called by
`CreateDatabaseCommand` and `AlterDatabasePropertiesCommand` right after
validation, rewrites the database property to its canonical name before it is
stored. This matters for compatibility: the stored map goes to the edit log and
the image, and an FE without this change (an old follower during a rolling
upgrade, or after a downgrade) parses it with `valueOf`. Had it been stored as
`row`, that FE would fail to replay it with the same `No enum constant`
exception. With the rewrite, the stored value is always one that an FE without
this change could have written itself.
What it buys:
- Any case is accepted by all four DDLs, and the stored and displayed value
is always upper case.
- Persisted metadata keeps exactly the old format, so mixed-version clusters
and downgrades are unaffected.
- An unknown value such as `rows` gets `Invalid binlog format value: rows`
from CREATE TABLE too, instead of the `No enum constant` message.
#### Results
"Before" is current master: the messages were reproduced by switching
`fromString` back to `valueOf` in the FE unit tests. "After" is this branch on
a local 1 FE + 1 BE cluster, checked by the new regression suite.
| Statement | Before | After |
|---|---|---|
| `CREATE TABLE ... ('binlog.enable' = 'true', 'binlog.format' = 'row')` |
`No enum constant org.apache.doris.catalog.BinlogConfig.BinlogFormat.row` | ROW
binlog table; `information_schema.table_properties` shows `ROW`; the `binlog()`
TVF returns the inserted rows |
| `CREATE TABLE ... ('binlog.enable' = 'true', 'binlog.format' =
'Statement_And_Snapshot')` | `No enum constant
...BinlogFormat.Statement_And_Snapshot` | CCR binlog table, stored as
`STATEMENT_AND_SNAPSHOT` |
| `ALTER TABLE <ROW table> SET ('binlog.format' = 'Row')` | `Invalid binlog
format value: Row` | accepted, no-op |
| `ALTER TABLE <ROW table> SET ('binlog.format' = 'statement_and_snapshot')`
| `Invalid binlog format value: statement_and_snapshot` | `not support change
binlog format from ROW to STATEMENT_AND_SNAPSHOT` (the real reason) |
| `CREATE DATABASE d PROPERTIES ('binlog.enable' = 'true', 'binlog.format' =
'row')` | `Invalid binlog format value: row` | created;
`information_schema.database_properties` shows `ROW`; a new table in `d`
inherits ROW binlog |
| `ALTER DATABASE d SET PROPERTIES ('binlog.enable' = 'false',
'binlog.format' = 'statement_and_snapshot')` | `Invalid binlog format value:
statement_and_snapshot` | accepted, stored as `STATEMENT_AND_SNAPSHOT` |
| `CREATE TABLE ... ('binlog.format' = 'rows')` | `No enum constant
...BinlogFormat.rows` | `Invalid binlog format value: rows` |
| ALTER TABLE / CREATE DATABASE / ALTER DATABASE with `'rows'` | `Invalid
binlog format value: rows` | unchanged |
#### Classes, and how they call each other
- `BinlogConfig.BinlogFormat#fromString` (new): the only parser of the value.
- `PropertyAnalyzer#analyzeBinlogConfig`: validates, now returns the
canonical name.
- `PropertyAnalyzer#normalizeBinlogFormat` (new): rewrites a map's value in
place; used only for database properties.
- `BinlogConfig#mergeFromProperties`, `TableProperty#buildBinlogConfig`:
switched from `valueOf` to `fromString`.
- `CreateDatabaseCommand#validate`,
`AlterDatabasePropertiesCommand#validate`: call `normalizeBinlogFormat` after
`analyzeBinlogConfig`.
```
CREATE TABLE InternalCatalog#createOlapTable
Database#getBinlogConfigsForCreateTable ->
BinlogConfig#mergeFromProperties -> fromString
PropertyAnalyzer#analyzeBinlogConfig
-> fromString
(the table stores BinlogConfig#toProperties, i.e. the
enum name)
ALTER TABLE ModifyTablePropertiesOp#validate ->
PropertyAnalyzer#analyzeBinlogConfig -> fromString
SchemaChangeHandler#updateBinlogConfig ->
analyzeBinlogConfig + mergeFromProperties
CREATE DATABASE CreateDatabaseCommand#validate ->
analyzeBinlogConfig, then normalizeBinlogFormat -> fromString
ALTER DATABASE AlterDatabasePropertiesCommand#validate ->
analyzeBinlogConfig, then normalizeBinlogFormat -> fromString
(the map is then stored in DatabaseProperty and journaled
as is)
replay / image Database#gsonPostProcess, #replayUpdateDbProperties
-> DatabaseProperty#getBinlogConfig ->
BinlogConfig#mergeFromProperties -> fromString
TableProperty#buildBinlogConfig
-> fromString
```
Not changed: BE, thrift and protobuf (they receive the enum, not the
string), the rule that a table's format is immutable, and every other
`binlog.*` property.
### Release note
The table and database property `binlog.format` is now case-insensitive, for
example `'binlog.format' = 'row'` is accepted. The value is always stored and
shown in upper case.
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [x] 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? -->
- FE UT: `BinlogFormatCaseInsensitiveTest` (new; CREATE / ALTER TABLE,
CREATE / ALTER DATABASE, unknown values, re-parsing stored values) and new
cases in `PropertyAnalyzerTest`. Together with `RowBinlogTtlConfigTest`,
`RowBinlogFeatureGateTest` and `RowBinlogConfigPublicationTest`: 56 tests, all
pass. With `fromString` switched back to `valueOf`, the 9 new case-insensitive
cases fail.
- Regression: `row_binlog_p0/test_binlog_format_case_insensitive` (new;
`.out` generated by the runner) passes on a local 1 FE + 1 BE cluster, and so
do the existing `test_binlog_property_alter_exception`,
`test_row_binlog_basic`, `test_row_binlog_cluster_key`,
`test_alter_database_properties_command` and `test_database_properties`.
- Behavior changed:
- [ ] No.
- [x] Yes. <!-- Explain the behavior change --> `binlog.format` values
in any case, which used to be rejected, are now accepted and stored by their
canonical upper case name. CREATE TABLE reports an unknown value as `Invalid
binlog format value: <value>` instead of `No enum constant ...`.
- 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]
