Repository navigation
[fix](fe) Keep rollup jobs waiting on conflict txn abort failure - #67661
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
This PR prevents rollup jobs from being cancelled when aborting conflict transactions fails due to the txn already being committed/visible/cleaned up, by retrying in subsequent scheduler rounds instead.
Changes:
- Skip committed/final transactions in
GlobalTransactionMgr.checkFailedTxns()to avoid unnecessary abort attempts. - Update
RollupJobV2to catch abort failures, log, and keep the job inWAITING_TXNfor retry. - Add/extend unit tests covering offline coordinators and commit-during-abort behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| fe/fe-core/src/main/java/org/apache/doris/transaction/GlobalTransactionMgr.java | Filters out committed/final txns from failed-txn selection to reduce abort attempts. |
| fe/fe-core/src/main/java/org/apache/doris/alter/RollupJobV2.java | Converts abort failure into a retry by logging and returning false (stay in WAITING_TXN). |
| fe/fe-core/src/test/java/org/apache/doris/transaction/GlobalTransactionMgrTest.java | Adds coverage for checkFailedTxns behavior with offline coordinators across txn states. |
| fe/fe-core/src/test/java/org/apache/doris/alter/RollupJobV2Test.java | Adds regression-style test for commit occurring while rollup attempts to abort. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Review status: complete after two convergence rounds; requesting changes for one blocking concurrency/lifecycle defect.
Critical checkpoints:
- Goal, correctness, and tests: The status filter correctly leaves COMMITTED as a wait-only state until VISIBLE and keeps PREPARE/PRECOMMITTED abortable. The added tests deterministically cover the callback-free commit race and final-status selection, but they do not cover callback cleanup; that gap exposes the blocking issue below.
- Scope and clarity: The four-file change is otherwise small and focused. The existing abort-reason and predicate-maintainability comments were duplicate-fenced rather than repeated.
- Concurrency and locking: The alter scheduler can race a load commit/publish. The local abort path runs callback setup before revalidating status under the transaction monitor and does not guarantee callback teardown on the resulting exception; see the inline finding.
- Lifecycle: Routine-load before/after transaction callbacks deliberately hand off a write lock. When commit wins this race, the new retry path can leave that lock permanently held.
- Configuration and parallel paths: No new configuration was added; the existing mutable gate is read each scheduler round. Schema-change and cloud-upgrade callers were reviewed. The shared selector is safe for their status semantics, and cloud abort already pairs its after-callback in a finally block.
- Persistence, data correctness, and compatibility: No persisted fields, edit-log formats, FE/BE protocol values, or storage formats change. COMMITTED transactions remain non-abortable and block progress until visibility. No additional issue was found here.
- Performance and observability: The status prefilter avoids unnecessary coordinator checks, and the new warning includes transaction and rollup job identifiers. No additional performance or observability issue was found.
- Test/CI evidence: The new tests use JUnit 5 and deterministic orchestration. Builds/tests were not run because this automated review explicitly prohibits them. The visible CheckStyle failure reports an unchanged file outside this PR's authoritative changed-file list.
- User focus: No additional user-provided focus was supplied; the full PR was reviewed.
### What problem does this PR solve? Issue Number: None Related PR: apache#65196 Problem Summary: A rollup job in WAITING_TXN checks and aborts failed conflict transactions before proceeding. If a transaction is already committed, becomes visible, or is cleaned up before the abort completes, abortTransaction() can throw UserException. RollupJobV2 propagates this as AlterCancelException, causing the entire rollup job to be cancelled. Restarting FE with committed but not yet visible transactions reproduces this failure. Apply the retry behavior introduced for schema change in apache#65196 to rollup jobs: log the abort failure, keep the job in WAITING_TXN, and re-query transaction state in the next scheduler round. Also exclude committed and final transactions from the shared failed-transaction selector to avoid unnecessary abort attempts. The exception handling remains necessary because transaction state can change after selection. ### Release note Prevent rollup and synchronous materialized view jobs from being cancelled when aborting a conflicting transaction fails. ### Check List (For Author) - Test: Unit Test / Regression test / Manual test - run-fe-ut.sh: GlobalTransactionMgrTest#testCheckFailedTxnsWithOfflineCoordinator and RollupJobV2Test#testCommitWhileAbortingPreviousLoad; 2 tests passed - run-regression-test.sh: insert_p0/transaction, txn_insert_restart_fe_with_schema_change, not_cloud mode; 1 suite passed, 0 skipped - BUILD_TYPE=ASAN ./build.sh --fe -j8 passed - FE Core Checkstyle: 0 violations; git diff --check passed - Behavior changed: Yes; rollup jobs wait and retry when conflict transaction abort fails, and committed or final transactions are excluded from automatic abort selection - Does this need documentation: No
|
run buildall |
TPC-H: Total hot run time: 16846 ms |
FE UT Coverage ReportIncrement line coverage |
TPC-DS: Total hot run time: 82377 ms |
ClickBench: Total hot run time: 14.82 s |
FE Regression Coverage ReportIncrement line coverage |
) Related PR: #65196 Problem Summary: A rollup job in WAITING_TXN checks and aborts failed conflict transactions before proceeding. If a transaction is already committed, becomes visible, or is cleaned up before the abort completes, abortTransaction() can throw UserException. RollupJobV2 propagates this as AlterCancelException, causing the entire rollup job to be cancelled. Restarting FE with committed but not yet visible transactions reproduces this failure (the txn_insert_restart_fe_with_schema_change regression case can produce this bug). Apply the retry behavior introduced for schema change in #65196 to rollup jobs: log the abort failure, keep the job in WAITING_TXN, and re-query transaction state in the next scheduler round. Also exclude committed and final transactions from the shared failed-transaction selector to avoid unnecessary abort attempts.
) ### What problem does this PR solve? Issue Number: close #68755 Related PR: #65196, #67661, #67761 Problem Summary: In Apache Doris 4.1.3, a cloud stream-load transaction can remain PREPARE after its coordinator BE is restarted or rescheduled. With FQDN addressing, the registered backend hostname differs from the recorded client IP; after rescheduling, the pod IP can also change. FE failure detection and meta-service coordinator cleanup then miss the abandoned transaction, leaving schema changes and rollups in WAITING_TXN until expiration or manual abort. This PR targets master and uses the backend ID when both coordinator IDs are nonzero, falling back to address matching when either ID is unknown. The cloud FE checks restart/liveness after backend lookup by ID without requiring hostname/IP equality. Startup-time and transaction-status filters remain in place, and non-cloud FE address matching is preserved. Master already contains the schema-change/rollup retry and committed/final-status safeguards from #65196 and #67661. This PR complements those changes and preserves the scan/retry behavior from #67761. Tests added cover cloud restart and lost-heartbeat detection with differing addresses, protection of the current process/recent heartbeat, non-cloud compatibility, changed-address lookup on both meta-service scan paths, startup-time filtering, different known IDs sharing an address, unknown request-ID fallback, and changed-address abort with current-process protection. Validation: - On this master-based patch: Java Checkstyle passed for both changed Java files; changed C++ files were formatted with clang-format 16.0.6; `git diff --check` passed. - Source-extracted compiled checks on this patch passed 33 FE failure-detection cases and 54 identity cases for each of the two meta-service matching expressions. These checks are isolated harnesses, not the project's native unit-test suites. - Earlier Docker validation for the 4.1.3 fix passed `AlterTxnRecoveryTest` (4 tests) and `CloudIndexTest`, `SchemaChangeJobV2Test`, `RollupJobV2Test`, and `GlobalTransactionMgrTest` (37 tests total). These results do not validate this master PR head. - The earlier full FE Docker run is still in progress. Two failures have been observed in `AuthenticationPluginManagerTest` concerning OIDC factory/plugin loading. The run uses `maven.test.failure.ignore=true` to collect failures across modules; a successful Maven exit must not be interpreted as a passing suite. - Native execution of the new master-based FE and cloud tests, full-suite validation of this PR head, and a cloud BE-rescheduling end-to-end reproduction are pending. No successful result is claimed for them. CI / review follow-up: request CI workflow approval from a Committer, then trigger `run buildall` for the current PR head according to the project's review-and-CI guide. Cloud/meta-service Code Owner review is required. This PR is ready for code review; pending validation and required approvals must be resolved before merge. ### Release note Fix cloud transaction cleanup missing restarted or rescheduled BE coordinators when their address changes or FQDN addressing is used, which could leave schema changes and rollups waiting for abandoned prepared transactions. ### Check List (For Author) - Test - [ ] 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 - Behavior changed: - [ ] No. - [x] Yes. Cloud cleanup matches known backend IDs despite address changes; unknown IDs fall back to address matching. - Does this need documentation? - [x] No. - [ ] Yes. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label
) ### What problem does this PR solve? Issue Number: close #68755 Related PR: #65196, #67661, #67761 Problem Summary: In Apache Doris 4.1.3, a cloud stream-load transaction can remain PREPARE after its coordinator BE is restarted or rescheduled. With FQDN addressing, the registered backend hostname differs from the recorded client IP; after rescheduling, the pod IP can also change. FE failure detection and meta-service coordinator cleanup then miss the abandoned transaction, leaving schema changes and rollups in WAITING_TXN until expiration or manual abort. This PR targets master and uses the backend ID when both coordinator IDs are nonzero, falling back to address matching when either ID is unknown. The cloud FE checks restart/liveness after backend lookup by ID without requiring hostname/IP equality. Startup-time and transaction-status filters remain in place, and non-cloud FE address matching is preserved. Master already contains the schema-change/rollup retry and committed/final-status safeguards from #65196 and #67661. This PR complements those changes and preserves the scan/retry behavior from #67761. Tests added cover cloud restart and lost-heartbeat detection with differing addresses, protection of the current process/recent heartbeat, non-cloud compatibility, changed-address lookup on both meta-service scan paths, startup-time filtering, different known IDs sharing an address, unknown request-ID fallback, and changed-address abort with current-process protection. Validation: - On this master-based patch: Java Checkstyle passed for both changed Java files; changed C++ files were formatted with clang-format 16.0.6; `git diff --check` passed. - Source-extracted compiled checks on this patch passed 33 FE failure-detection cases and 54 identity cases for each of the two meta-service matching expressions. These checks are isolated harnesses, not the project's native unit-test suites. - Earlier Docker validation for the 4.1.3 fix passed `AlterTxnRecoveryTest` (4 tests) and `CloudIndexTest`, `SchemaChangeJobV2Test`, `RollupJobV2Test`, and `GlobalTransactionMgrTest` (37 tests total). These results do not validate this master PR head. - The earlier full FE Docker run is still in progress. Two failures have been observed in `AuthenticationPluginManagerTest` concerning OIDC factory/plugin loading. The run uses `maven.test.failure.ignore=true` to collect failures across modules; a successful Maven exit must not be interpreted as a passing suite. - Native execution of the new master-based FE and cloud tests, full-suite validation of this PR head, and a cloud BE-rescheduling end-to-end reproduction are pending. No successful result is claimed for them. CI / review follow-up: request CI workflow approval from a Committer, then trigger `run buildall` for the current PR head according to the project's review-and-CI guide. Cloud/meta-service Code Owner review is required. This PR is ready for code review; pending validation and required approvals must be resolved before merge. ### Release note Fix cloud transaction cleanup missing restarted or rescheduled BE coordinators when their address changes or FQDN addressing is used, which could leave schema changes and rollups waiting for abandoned prepared transactions. ### Check List (For Author) - Test - [ ] 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 - Behavior changed: - [ ] No. - [x] Yes. Cloud cleanup matches known backend IDs despite address changes; unknown IDs fall back to address matching. - Does this need documentation? - [x] No. - [ ] Yes. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label
Related PR: #65196
Problem Summary:
A rollup job in WAITING_TXN checks and aborts failed conflict transactions before proceeding. If a transaction is already committed, becomes visible, or is cleaned up before the abort completes, abortTransaction() can throw UserException. RollupJobV2 propagates this as AlterCancelException, causing the entire rollup job to be cancelled. Restarting FE with committed but not yet visible transactions reproduces this failure (the txn_insert_restart_fe_with_schema_change regression case can produce this bug).
Apply the retry behavior introduced for schema change in #65196 to rollup jobs: log the abort failure, keep the job in WAITING_TXN, and re-query transaction state in the next scheduler round. Also exclude committed and final transactions from the shared failed-transaction selector to avoid unnecessary abort attempts.