Uh oh!
There was an error while loading. Please reload this page.
[fix](storage) Reject prepare txn on shutdown tablet - #66448
Conversation
Cause: PR apache#54124 moved the migration and push locks from RowsetBuilder into Tablet::prepare_txn. During that refactor, the call to TxnManager::prepare_txn changed from the const Tablet& overload to the raw tablet_id/tablet_uid overload. This silently bypassed the TABLET_SHUTDOWN guard introduced by PR apache#42296. Impact: A load thread holding an old Tablet instance could resume after migration and register the stale tablet UID in the transaction map. Publish would then operate on the reloaded Tablet with a new UID and leave the transaction in an E-909 state. Fix: Call the const Tablet& overload with *this while the migration and push locks are held. This preserves the lock encapsulation from PR apache#54124 and restores the shutdown-tablet protection from PR apache#42296. Test: Add a deterministic unit test that marks a Tablet as TABLET_SHUTDOWN, verifies Tablet::prepare_txn fails, and verifies no stale tablet entry is registered. TxnManagerTest.* passes 16/16. (cherry picked from commit ccfb166677719b65dd224970e0f2323a1e93c1fb)
hello-stephen
commented
Aug 4, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
deardeng
commented
Aug 4, 2026
run buildall |
deardeng
commented
Aug 4, 2026
/review |
hello-stephen
commented
Aug 4, 2026
TPC-H: Total hot run time: 28715 ms |
hello-stephen
commented
Aug 4, 2026
TPC-DS: Total hot run time: 165875 ms |
hello-stephen
commented
Aug 4, 2026
ClickBench: Total hot run time: 23.98 s |
There was a problem hiding this comment.
Automated review result: request changes
The overload switch correctly restores the shutdown guard for ordinary Tablet::prepare_txn callers, but two P1 lifecycle gaps can still record a retired tablet UID and fail exact-UID publish.
Critical checkpoint conclusions:
- Goal and correctness: the changed path fixes the sequential overload-dispatch regression, but the end-to-end stale-tablet goal remains incomplete because the slave direct-commit path bypasses prepare and retained tablets are not fenced through commit against other shutdown/replacement paths.
- Scope and parallel paths: the patch is minimal and focused;
RowsetBuilder, realtime push, and backend-binlog ingestion converge on the guarded path and propagate itsStatus, while the slave pull is the relevant live bypass. - Concurrency and lifecycle: the intended disk-migration lock ordering is consistent and no new deadlock was found. Bad-tablet repair, manual reload/replacement, and one-shot FE drop cleanup do not provide an equivalent prepare-to-commit/current-object fence.
- Transactions, persistence, and data correctness: the remaining interleavings can persist or insert an old
(tablet_id, tablet_uid)identity; publish later requires that exact identity and cannot resolve the replacement. - Tests: the added unit test deterministically proves overload dispatch and absence of a prepared map entry for an already-shutdown tablet. It does not cover slave-pull/migration or retained-pointer shutdown/prepare/commit interleavings. No local builds or tests were run because the authoritative review prompt requires static-only review.
- Error handling and observability: ordinary callers already propagate the failure, and the guarded error identifies the tablet; no additional observability issue was found.
- Configuration, compatibility, ownership, memory, and performance: no applicable regression was found; the restored state check is constant-time.
- Review coverage: no additional user focus was supplied, the full two-file diff was reviewed, and no existing inline thread duplicates either finding.
- Completion: three review rounds were used; all three final-round reviewers returned
NO_NEW_VALUABLE_FINDINGSagainst these exact comments and diff position 6.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
PR approved by at least one committer and no changes requested. |
PR approved by anyone and no changes requested. |
hello-stephen
commented
Aug 5, 2026
skip check_coverage |
Uh oh!
There was an error while loading. Please reload this page.
Cause: PR #54124 moved the migration and push locks from RowsetBuilder into Tablet::prepare_txn. During that refactor, the call to TxnManager::prepare_txn changed from the const Tablet& overload to the raw tablet_id/tablet_uid overload. This silently bypassed the TABLET_SHUTDOWN guard introduced by PR #42296. Impact: A load thread holding an old Tablet instance could resume after migration and register the stale tablet UID in the transaction map. Publish would then operate on the reloaded Tablet with a new UID and leave the transaction in an E-909 state. Fix: Call the const Tablet& overload with *this while the migration and push locks are held. This preserves the lock encapsulation from PR #54124 and restores the shutdown-tablet protection from PR #42296. Test: Add a deterministic unit test that marks a Tablet as TABLET_SHUTDOWN, verifies Tablet::prepare_txn fails, and verifies no stale tablet entry is registered. TxnManagerTest.* passes 16/16.
Cause: PR #54124 moved the migration and push locks from RowsetBuilder into Tablet::prepare_txn. During that refactor, the call to TxnManager::prepare_txn changed from the const Tablet& overload to the raw tablet_id/tablet_uid overload. This silently bypassed the TABLET_SHUTDOWN guard introduced by PR #42296. Impact: A load thread holding an old Tablet instance could resume after migration and register the stale tablet UID in the transaction map. Publish would then operate on the reloaded Tablet with a new UID and leave the transaction in an E-909 state. Fix: Call the const Tablet& overload with *this while the migration and push locks are held. This preserves the lock encapsulation from PR #54124 and restores the shutdown-tablet protection from PR #42296. Test: Add a deterministic unit test that marks a Tablet as TABLET_SHUTDOWN, verifies Tablet::prepare_txn fails, and verifies no stale tablet entry is registered. TxnManagerTest.* passes 16/16.
Cause: PR apache#54124 moved the migration and push locks from RowsetBuilder into Tablet::prepare_txn. During that refactor, the call to TxnManager::prepare_txn changed from the const Tablet& overload to the raw tablet_id/tablet_uid overload. This silently bypassed the TABLET_SHUTDOWN guard introduced by PR apache#42296. Impact: A load thread holding an old Tablet instance could resume after migration and register the stale tablet UID in the transaction map. Publish would then operate on the reloaded Tablet with a new UID and leave the transaction in an E-909 state. Fix: Call the const Tablet& overload with *this while the migration and push locks are held. This preserves the lock encapsulation from PR apache#54124 and restores the shutdown-tablet protection from PR apache#42296. Test: Add a deterministic unit test that marks a Tablet as TABLET_SHUTDOWN, verifies Tablet::prepare_txn fails, and verifies no stale tablet entry is registered. TxnManagerTest.* passes 16/16.
Cause: PR #54124 moved the migration and push locks from RowsetBuilder into Tablet::prepare_txn. During that refactor, the call to TxnManager::prepare_txn changed from the const Tablet& overload to the raw tablet_id/tablet_uid overload. This silently bypassed the TABLET_SHUTDOWN guard introduced by PR #42296. Impact: A load thread holding an old Tablet instance could resume after migration and register the stale tablet UID in the transaction map. Publish would then operate on the reloaded Tablet with a new UID and leave the transaction in an E-909 state. Fix: Call the const Tablet& overload with *this while the migration and push locks are held. This preserves the lock encapsulation from PR #54124 and restores the shutdown-tablet protection from PR #42296. Test: Add a deterministic unit test that marks a Tablet as TABLET_SHUTDOWN, verifies Tablet::prepare_txn fails, and verifies no stale tablet entry is registered. TxnManagerTest.* passes 16/16.
Cause:
PR #54124 moved the migration and push locks from RowsetBuilder into Tablet::prepare_txn. During that refactor, the call to TxnManager::prepare_txn changed from the const Tablet& overload to the raw tablet_id/tablet_uid overload. This silently bypassed the TABLET_SHUTDOWN guard introduced by PR #42296.
Impact:
A load thread holding an old Tablet instance could resume after migration and register the stale tablet UID in the transaction map. Publish would then operate on the reloaded Tablet with a new UID and leave the transaction in an E-909 state.
Fix:
Call the const Tablet& overload with *this while the migration and push locks are held. This preserves the lock encapsulation from PR #54124 and restores the shutdown-tablet protection from PR #42296.
Test:
Add a deterministic unit test that marks a Tablet as TABLET_SHUTDOWN, verifies Tablet::prepare_txn fails, and verifies no stale tablet entry is registered. TxnManagerTest.* passes 16/16.
(cherry picked from commit ccfb166677719b65dd224970e0f2323a1e93c1fb)
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)