[fix](fe) Keep CDC end offset consistent with current progress#65688
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes streaming CDC job end-offset reporting in FE so that the published EndOffset cannot lag behind the committed CurrentOffset when the task has progressed beyond a previously fetched end offset. It also adds targeted FE unit coverage and hardens a PostgreSQL CDC regression case by waiting for the first committed task before inserting new rows.
Changes:
- Preserve the three-way offset comparison result and, when the fetched end is behind, advance
EndOffsetto the committed current binlog offset. - Synchronize end-offset publication so a stale compare result can’t overwrite a newer fetched end offset.
- Add FE unit tests and adjust the Postgres “latest offset” regression test to wait for the first successful task commit.
Review Checkpoints (Part 1.3)
- Goal & correctness: The intent is clear and is backed by a new FE unit test class plus a regression timing adjustment; one correctness issue remains in
compareOffset()(see PR comment). - Scope/focus: Changes are focused to CDC offset comparison + synchronization and associated tests.
- Concurrency: New locking around
endBinlogOffset/hasMoreDatalooks consistent with existingsplitsLockusage, and heavy RPC work remains outside the lock. - Test coverage: Added FE unit coverage for the new comparison behavior and a regression stabilization for Postgres “latest offset”.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| regression-test/suites/job_p0/streaming_job/cdc/tvf/test_streaming_job_cdc_stream_postgres_latest_alter_cred.groovy | Waits for first successful task commit (resolved latest offset) before inserting new rows to reduce flakiness. |
| fe/fe-core/src/test/java/org/apache/doris/job/offset/jdbc/JdbcSourceOffsetProviderOffsetTest.java | Adds FE unit tests to validate end-offset advancement and stale-compare overwrite protection. |
| fe/fe-core/src/main/java/org/apache/doris/job/offset/jdbc/JdbcSourceOffsetProvider.java | Implements three-way compare handling, advances EndOffset when behind, and synchronizes end-offset publication. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
390
to
+392
| Integer cmp = parseCdcResponseData( | ||
| result.getResponse(), new TypeReference<Integer>() {}); | ||
| return cmp != null && cmp > 0; | ||
| return cmp; |
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
TPC-H: Total hot run time: 29978 ms |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Automated review summary
Overall: Request changes. The signed comparison handles the direct CurrentOffset > EndOffset case correctly, but asynchronous end-offset publication is still unsafe in both completion orders described in the inline comment.
Critical checkpoint conclusions:
- Goal and proof: argument order/sign are correct and the new unit tests prove the positive/negative paths plus stale compare/current replacement. They do not cover an unchanged zero result, equal-valued refresh, or stale fetch completion, so the intended invariant is not fully proved.
- Scope and clarity: the production changes are focused on CDC offset publication and mirror the binlog commit update in both base and TVF providers.
- Concurrency: metadata fetch, task scheduling/compare, and transaction/ALTER/replay publication run on independent paths; BE forwarding is concurrent. RPCs remain outside the single
splitsLock, with no new deadlock or heavy-under-lock issue, but fetch responses lack the generation/state validation applied to compare responses. - Lifecycle and persistence: committed current progress remains covered by existing
bop/attachment/checkpoint and cloud/non-cloud replay paths; fetched EndOffset is intentionally transient. No static initialization, reference-cycle, or release issue was found. - Configuration and compatibility: no config, persisted format, or wire payload changed.
/api/compareOffsetstill returnsfirst.compareTo(second), and sign-only handling is compatible with non-canonical comparator magnitudes. - Parallel paths and conditions: base/TVF and shared-nothing/cloud current-offset paths are aligned; sign branches are otherwise correct. The successful-null response issue at line 404 is already covered by the existing Copilot thread and is not duplicated here.
- Tests and results:
SucceedTaskCount >= 1is ordered after the transaction callback publishes the resolved latest offset, so the regression readiness gate is valid; no expected-result file changed. The two missing concurrency cases should be added with the fix. - Observability and performance: existing RPC logs/metrics are adequate. The equal-value reference churn can defeat idle backoff and run empty loads.
- Transactions, data, and protocol state: no table-data visibility, delete-bitmap, memory-accounting, or new FE-BE variable issue applies. Current-offset publication occurs before task success accounting.
- Validation: static review only, as the review-runner instructions prohibit builds and local test execution.
User focus: no additional focus was provided; the full PR was reviewed.
| if (endBinlogOffset == null || !endBinlogOffset.equals(newEndOffset)) { | ||
| hasMoreData = true; | ||
| } | ||
| endBinlogOffset = newEndOffset; |
Contributor
There was a problem hiding this comment.
[P2] Make fetch publication participate in stale-state validation
Putting the assignment under splitsLock does not ensure that the asynchronously fetched value is current, and it interacts badly with the new reference-identity guard in both directions:
- If a compare snapshots
E1, an unchanged periodic fetch is deserialized asE2; this assignment replaces the reference even though the values are equal. A valid zero comparison then looks stale, returnstrue, and can dispatch an empty load instead of idling. - If a fetch starts at
E, then a task commitsC > Eand the negative compare branch promotes EndOffset toC, the older fetch can finish afterward and lower EndOffset back toE, recreating the exactCurrentOffset > EndOffsetstate this change is fixing.
The BE forwards these requests on a concurrent heavy-work pool, so both completion orders are possible. Please snapshot/version current and end state around fetchEndOffset, preserve the existing map reference for a value-equal response, and reject a response made stale by intervening progress. The focused tests should cover both the equal-replacement and reverse-completion interleavings.
Member
Author
There was a problem hiding this comment.
This interval will be very short; the next correction will take place after 500ms.
Contributor
TPC-DS: Total hot run time: 178145 ms |
Contributor
ClickBench: Total hot run time: 25.44 s |
Member
Author
|
run p0 |
github-actions Bot
pushed a commit
that referenced
this pull request
Jul 20, 2026
### What problem does this PR solve? Streaming CDC jobs periodically fetch the source end offset, while a successful task commits its current offset independently. A task can advance beyond the previously fetched end offset. The FE previously converted the offset comparison into a boolean, so it could stop scheduling but could not distinguish equality from the current offset being ahead, leaving CurrentOffset greater than EndOffset in job output. This change preserves the three-way comparison result and advances EndOffset to CurrentOffset when the fetched end is behind. End-offset publication is synchronized so an obsolete comparison result cannot overwrite a newer fetched end. The PostgreSQL latest-offset credential regression case now waits for the first task to commit the resolved latest offset before inserting new rows.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
Streaming CDC jobs periodically fetch the source end offset, while a successful task commits its current offset independently. A task can advance beyond the previously fetched end offset. The FE previously converted the offset comparison into a boolean, so it could stop scheduling but could not distinguish equality from the current offset being ahead, leaving CurrentOffset greater than EndOffset in job output.
This change preserves the three-way comparison result and advances EndOffset to CurrentOffset when the fetched end is behind. End-offset publication is synchronized so an obsolete comparison result cannot overwrite a newer fetched end. The PostgreSQL latest-offset credential regression case now waits for the first task to commit the resolved latest offset before inserting new rows.
Release note
Keep the displayed streaming CDC end offset from falling behind committed current progress after offset comparison.
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)