morningman opened a new pull request, #69242:
URL: https://github.com/apache/doris/pull/69242
### What problem does this PR solve?
Issue Number: None
Related PR: #65810
Problem Summary:
**In short.** Row binlog tablets keep the positions of their three hidden
columns (`__DORIS_BINLOG_TSO__`, `__DORIS_BINLOG_LSN__`, `__DORIS_BINLOG_OP__`)
in `TabletSchema`. Since #65810 the FE no longer sends them, so every row
binlog tablet created since then has -1 in its tablet meta. Nothing reads that
-1 today, but any new reader would. Only the BE unit tests still fill these
fields, and that takes them down a path production never runs. This PR makes
the BE derive the three positions from the column names whenever a schema is
loaded. It also retires the thrift and protobuf fields that used to carry them.
Nothing that runs today changes.
**Background.**
- A row binlog tablet stores change events. Besides the user columns, its
schema has three hidden columns, `__DORIS_BINLOG_TSO__`, `__DORIS_BINLOG_LSN__`
and `__DORIS_BINLOG_OP__`. `TabletSchema` keeps their positions in
`_binlog_{tso,lsn,op}_col_idx`.
- The positions came from thrift `TTabletSchema` 28-30 when a tablet was
created, and were persisted in `TabletSchemaPB` 41-43 (`TabletSchemaCloudPB`
42-44).
- Schemas built column by column already derive them from the names:
`TabletSchema::append_column` and `build_current_tablet_schema`. Only
`TabletSchema::init_from_pb`, which loads a schema from its PB, took the stored
values.
**The problem.**
- #65810 stopped the FE from setting `TTabletSchema` 28-30
(`CreateReplicaTask`, and `CloudInternalCatalog` for cloud). Every row binlog
tablet created since then has -1 in:
- its tablet meta schema;
- the meta service schema entry, in cloud mode;
- every schema loaded from those, such as cloud rowsets synced from the
meta service and compaction outputs.
- Nothing reads these -1 today. Every reader gets a schema whose positions
are derived from the names: loads (`build_current_tablet_schema`), scans and
the publish-phase writer (`append_column`), and the rowset written by the same
load. Still:
1. Any new code that reads the positions from the tablet schema gets -1.
2. If a load ever came with an index schema without column unique ids,
`RowsetBuilder` would fall back to copying the tablet schema, and row binlog
derivation would hit `CHECK(idx >= 0)`.
3. The BE unit tests (`create_row_binlog_tablet_schema` and others)
still set 28-30 or the PB fields by hand, so they exercise a path production
never takes.
**How this PR fixes it.**
1. `TabletSchema::init_from_pb` derives the three positions with
`field_index(<column name>)`, as `append_column` does. A schema now has the
same positions whether it was built column by column or loaded from a PB.
2. The stored values are no longer needed, so the fields are retired:
| File | Message | Ids | Old fields |
|---|---|---|---|
| AgentService.thrift | `TTabletSchema` | 28, 29, 30 | `binlog_tso_idx`,
`binlog_lsn_idx`, `binlog_op_idx` (commented out) |
| olap_file.proto | `TabletSchemaPB` | 41, 42, 43 | `binlog_tso_col_idx`,
`binlog_lsn_col_idx`, `binlog_op_col_idx` (`reserved`) |
| olap_file.proto | `TabletSchemaCloudPB` | 42, 43, 44 | same (`reserved`) |
`TabletSchema::to_schema_pb`, `TabletMeta` and the cloud `pb_convert`
stop writing and copying them.
3. The BE unit tests stop setting them. A new
`TabletSchemaTest.test_binlog_col_idx_from_pb_column_names` checks the
positions:
- right after `init_from_pb`;
- after a round trip through the PB;
- on a schema without these columns, where they stay -1.
**Compatibility.**
- None of these ids exists on branch-4.2, branch-4.1, branch-4.0 or
branch-3.1.
- Meta written by an older master still loads. Protobuf keeps the reserved
ids as unknown fields, and the positions are derived anyway.
- An older master BE that reads meta written by this one gets -1, as it
already does for every row binlog tablet created since #65810.
- With the positions derived, `is_tso_enabled()` is now also true on a row
binlog tablet's own schema. Two callers read it from the tablet schema, and
neither changes behavior:
- `CloudTablet::try_make_committed_rs_visible` returned without doing
anything for a row binlog tablet before, and it still does. Row binlog tablets
are not merge-on-write and never put rowsets into the committed rowset manager.
- The cloud delete task never reaches a row binlog tablet, because
predicate DELETE is rejected on these tables.
### Release note
None
### 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? -->
- Build: `build.sh --fe --be` (BE Release; FE with checkstyle).
- Regression test: `row_binlog_p0` (23 suites) and `table_stream_p0` (16
suites) pass.
- Unit Test, BE (`run-be-ut.sh`), 191 tests in 19 suites, all pass:
- the new
`TabletSchemaTest.test_binlog_col_idx_from_pb_column_names`;
- `TabletSchemaTest`, `TabletMetaTest`, `PbConvert`;
- every suite that builds a row binlog schema:
`RowBinlogDeriveTest`, `RowBinlogVmergeCompactionTest`,
`SegmentFlusherFormatTest`, `SegmentFlusherTransformFormatTest`,
`GroupRowsetBuilderTest`, `GroupRowsetWriterTest`,
`CloudGroupRowsetBuilderWriterTest`, `TestEngineStorageMigrationTask`;
- the merge-on-write transform suites: `HistoricalRowFetcherTest`,
`KeyProbeTest`, `RowCacheProbeTest`, `HistoricalRowRetrieverTest`,
`FixedPartialUpdateTest`, `FlexiblePartialUpdateTest`, `ValidateStageTest`,
`VariantRowStoreTest`.
- Manual test, local mode:
1. Create a row binlog table and load it. The row binlog tablet meta
(`/api/meta/header`) no longer has the three fields.
2. Restart the cluster and load again.
3. Run a binlog compaction (`[0-1]` ... `[5-5]` into `[0-5]`).
4. Read through `binlog()`: every event comes back.
- Behavior changed:
- [x] No.
- [ ] Yes. <!-- Explain the behavior change -->
- 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]
