[feature](binlog) Keep row binlog tablets colocated with base tablets - #66191
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
22a745e to
25fc92b
Compare
|
run buildall |
TPC-H: Total hot run time: 28559 ms |
TPC-DS: Total hot run time: 169512 ms |
ClickBench: Total hot run time: 23.86 s |
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
AntiTopQuark
changed the title
| // Cross-link for a split base/row-binlog tablet pair. Legacy images do not | ||
| // contain this field and therefore keep the non-positive "not aligned" value. | ||
| @SerializedName(value = "ati") | ||
| protected long alignedTabletId = -1L; |
There was a problem hiding this comment.
This introduces a second, incompatible representation of the base/row-binlog tablet relationship. PR #65810 already persists directional links: base.rowBinlogTabletId and companion.rowBinlogBaseTabletId, and its creation, sink, and report paths use those fields. This PR instead adds alignedTabletId and makes the locality scheduler depend on it. When the two changes are combined, tablets created by #65810 leave alignedTabletId unset, so the scheduler cannot resolve the pair. Please reuse #65810’s directional fields as the canonical model, or provide an explicit compatibility/migration layer instead of introducing a parallel persisted field.
There was a problem hiding this comment.
Thanks, this has been addressed in the latest revision (d0182ef9ef) after rebasing on #65810.
This PR no longer introduces alignedTabletId / ati, separate row-binlog index states, or any other persisted tablet-pair representation. The locality, checker, and scheduler paths now use #65810’s canonical directional links:
- companion -> base:
getRowBinlogBaseTabletId() - base -> companion:
getRowBinlogTabletId()
The companion index is identified by MaterializedIndex.isRowBinlog() with IndexState.NORMAL. The current Tablet.java diff does not add any pairing field; it only adds the binlogMissing replica filter. No new Gson or wire representation is introduced by this PR.
The Gson and locality tests were also updated for the directional fields and for old JSON without those optional fields. Since alignedTabletId never landed in the community branch and no deployed image was written with that model, there is no metadata migration to perform. If a declared row-binlog companion has missing or inconsistent directional links, it is treated as invalid metadata rather than falling back to a second representation.
FE unit tests have not been run locally yet; only static source, conflict, and history checks were completed. Could you please take another look?
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: Row-binlog companion replicas can drift away from their base replicas during FE repair scheduling. This change evaluates companion health against effective base replicas, carries required backend and storage-path constraints into repair scheduling, protects complete replica pairs during deletion, and keeps ordinary colocate balancing statistics focused on queryable indexes. ### Release note Keep row-binlog companion tablets on the same backend and storage path as their base tablets during FE repair scheduling. ### Check List (For Author) - Test: Not run (per request; static conflict, source, and history checks only) - Behavior changed: Yes. FE repair scheduling now preserves base and row-binlog backend and path locality. - Does this need documentation: No
a231d97 to
d0182ef
Compare
| } | ||
|
|
||
| @Override | ||
| public void incrBinlogMissingCount() { |
There was a problem hiding this comment.
consumeBinlogMissingRetry() or decrementBinlogMissingCount()？
|
[blocker] The row-binlog storage migration can race with an in-flight group load. The row-binlog builder retains a tablet shared pointer but does not register a separate transaction for that tablet. get_tablet_related_txns() only checks transaction-map keys and therefore cannot see either an active base-keyed group transaction or its nested attach_row_binlog. Migration reload replaces the tablet with a new UID/object, while commit/publish continues using the retained old object. Please add pair-aware transaction draining or lifetime migration locking, together with tests covering both pre-commit and committed-but-not-published windows. |
|
run buildall |
TPC-H: Total hot run time: 16815 ms |
TPC-DS: Total hot run time: 82287 ms |
ClickBench: Total hot run time: 14.69 s |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
if (!is_related) {
for (const auto& tablet_txn : it.second) {
const auto& attached_tablet = tablet_txn.second->attach_row_binlog.tablet;
if (attached_tablet != nullptr &&
attached_tablet->tablet_id() == tablet_info.tablet_id &&
attached_tablet->tablet_uid() == tablet_info.tablet_uid) {
is_related = true;
break;
}
}
}Because EngineStorageMigrationTask::_check_running_txns() relies on this call, the migration now correctly detects the active base-keyed group transaction through its nested attach_row_binlog, and blocks/waits until the group load drains — no reload can swap the UID/object out from under an in-flight commit or publish. |
|
@gavinchou @morningman please review this PR, thx~ |
|
@yiguolei Hi, Please review this PR, thank you very much! |
|
PR approved by anyone and no changes requested. |
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
8 participants
What problem does this PR solve?
Issue Number: close #65418
Related PR: #xxx
Problem Summary:
Applying the row binlog tablet locality patch series left duplicate hunks, rejected FE patch files, a missing scheduler exception declaration, and stale regression cases. Reconcile the final patch state so row binlog tablets use the existing colocate mismatch scheduling path to stay on the same backend and disk path as their paired base tablets without joining user colocate groups.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)