morningman opened a new pull request, #69238:
URL: https://github.com/apache/doris/pull/69238
### What problem does this PR solve?
Issue Number: None
Related PR: #62058, #63643, #63850, #64133, #65810
Problem Summary:
**In short.** Row binlog (`binlog.format = 'ROW'`) was reworked several
times on master. The reworks left dead code behind, and they deleted thrift /
protobuf fields without retiring their ids. This PR deletes the dead code and
retires the ids: `reserved` in protobuf, a `// N: deprecated <name>` comment in
thrift. After this, no later change can reuse an id that an older master build
may have persisted or sent. Nothing that runs today changes. This is also
groundwork for renaming row binlog to changelog: code deleted here does not
have to be renamed there.
**Background.**
- #62058 and #63643 first kept row binlog inside the base tablet. The tablet
meta carried its own binlog schema and rowset metas (`row_binlog_schema`,
`row_binlog_rs_metas`, `row_binlog_schema_hash`). Delete bitmaps had an
`is_binlog_delvec` flag. Clone and snapshot requests had a `copy_type`. Tablet
reports carried `binlog_size` / `binlog_file_num`, which the FE kept on
`Replica`.
- #63850 added `binlog_timestamp_col_idx` / `binlog_lsn_col_idx` to the
tablet schema. #64133 replaced them with `binlog_{tso,lsn,op}_col_idx` under
new ids (41-43, and 42-44 in the cloud message). So the name
`binlog_lsn_col_idx` is still in use, but under a new id.
- #65810 moved row binlog into an independent companion tablet. Most of the
per-tablet fields above were deleted then. #65810 also added a thrift
`TTabletWithPartition.binlog_tablet_id`, which nothing uses.
**The problem.**
- Ten protobuf ids and six thrift ids were deleted without being retired.
`gensrc/AGENTS.md` requires a deleted id to be retired so it is never reused.
Protobuf is the risky case: these messages are persisted in tablet meta (local
RocksDB, and the cloud meta-service). Suppose a later field took one of these
ids with another type or meaning. A tablet meta written by an older master
build could then be decoded into the new field.
- Some code that nothing reaches is still there. FE `Replica` still carries
`binlogSize` / `binlogFileNum` and writes them into the image, though nothing
has set them since #65810. The thrift `TTabletWithPartition.binlog_tablet_id`
was never used. The BE has unused constants, a method and two error codes.
Without this PR, the changelog rename would have to rename all of it.
**How this PR fixes it.**
Dead code removed:
| Item | Why it is dead |
|---|---|
| thrift `TTabletWithPartition.binlog_tablet_id` (3) |
`TTabletWithPartition` only lives inside the BE (`VTabletWriter`); field 3 is
never set or read. The load path carries the companion tablet id in the
protobuf `PTabletWithPartition.binlog_tablet_id`, which stays. |
| FE `Replica.binlogSize` / `binlogFileNum` (Gson `bs` / `bfn`) and their
getters / setters | Their only writers, the tablet report fields, were removed
in #65810; nothing reads them. Gson skips the two keys when it loads an image
written by an older master. |
| BE `kRowBinlogPrefix`, `FDRowBinlogSuffix`, `Tablet::enable_row_binlog()`
| No references. |
| BE error codes `BINLOG_COMPACTION_INVALID_PARAMETERS` (-2011),
`BINLOG_COMPACTION_NO_SUITABLE_VERSION` (-2012) | No references. Removed the
same way -2007 was, without a placeholder. |
Retired ids:
| File | Message | Ids | Old fields |
|---|---|---|---|
| olap_file.proto | `TabletMetaPB` | 42, 43, 44 | `row_binlog_schema`,
`row_binlog_rs_metas`, `row_binlog_schema_hash` |
| olap_file.proto | `TabletMetaCloudPB` | 43, 44 | `row_binlog_rs_metas`,
`row_binlog_schema_hash` |
| olap_file.proto | `TabletSchemaPB` / `TabletSchemaCloudPB` | 37, 38 / 38,
39 | `binlog_timestamp_col_idx`, `binlog_lsn_col_idx` (the old ids only; see
Background) |
| olap_file.proto | `DeleteBitmapPB` | 5 | `is_binlog_delvec` |
| AgentService.thrift | `TCloneReq` / `TSnapshotRequest` | 14 / 15 |
`copy_type` |
| BackendService.thrift | `TTabletStat` | 12, 13 | `binlog_size`,
`binlog_file_num` |
| MasterService.thrift | `TTabletInfo` | 26, 27 | `binlog_size`,
`binlog_file_num` |
| PaloInternalService.thrift | `TTabletWithPartition` | 3 |
`binlog_tablet_id` |
None of these ids is used on branch-4.2, branch-4.1, branch-4.0 or
branch-3.1 (checked with `git show` on each branch), so retiring them agrees
with every release. `TabletMetaCloudPB` 42 is not in the table: it already
means something else on master (it was `row_binlog_schema`, it is now
`binlog_tablet_id`). This PR cannot undo that.
Not included: `TTabletSchema.binlog_{tso,lsn,op}_idx` (28-30). The FE
stopped sending them in #65810, but BE unit tests still set them when they
create a companion tablet. Whether production needs them is a separate
question, left to a follow-up.
**Results.**
- No behavior change. The one thrift field deleted here was never set, the
other thrift changes are comments, and `reserved` only makes protoc reject a
future reuse of the id.
- A peer built from an older master that still sends one of the deleted
thrift fields has it skipped as an unknown field, as it already is today. An FE
image that still has `bs` / `bfn` loads as before.
### Release note
None
### 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? -->
- Build: `build.sh --fe --be` (BE Release; FE with checkstyle).
- FE unit tests (`run-fe-ut.sh`): `ReplicaTest` (5), `TabletTest` (11),
`OlapTableTest` (18), all pass.
- No BE unit test: nothing under `be/test` references the removed
symbols (`git grep`).
- Cloud was not built locally. It only sees the new `reserved` lines,
which do not change the generated code.
- 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]
