Doris-Breakwater commented on issue #70280: URL: https://github.com/apache/doris/issues/70280#issuecomment-6654243323
Initial triage: **the FE handling discrepancy is confirmed, but permanent read loss is not reproduced, and the proposed raw-response injection does not model the normal Meta Service wire response.** This should be investigated together with the existing `KV_TXN_COMMIT_ERR` fencing work in [PR #69925](https://github.com/apache/doris/pull/69925), which is currently open and unmerged. I inspected `72e91896bbdd8760f8bd4e488e5672406b40ce52` and confirmed that the relevant FE cleanup and MS response-conversion blocks are unchanged on current master `bc9686578a5a3675b8d04b0a0b4fc9ff6a983d96`. The issue currently has no labels. **Verified code behavior** - The exception handler calls `fenceAndAbandonCommitTso()`, while a returned `KV_TXN_MAYBE_COMMITTED` calls `abandonCommitTso()`. A subsequent `UserException` is thrown outside that exception handler, so it does not fence retroactively. This contradicts the stated uncertain-result contract in [#67820](https://github.com/apache/doris/pull/67820). See [FE result handling](https://github.com/apache/doris/blob/72e91896bbdd8760f8bd4e488e5672406b40ce52/fe/fe-core/src/main/java/org/apache/doris/cloud/transaction/CloudGlobalTransactionMgr.java#L951-L1005). - This is also encoded in the existing test `testRpcFailureIsFencedButMaybeCommittedIsReleased`, which explicitly verifies that a maybe-committed response does **not** call the fencing helper. That test establishes the expected implementation behavior, not its safety. See [CloudCommittedTsoTest](https://github.com/apache/doris/blob/72e91896bbdd8760f8bd4e488e5672406b40ce52/fe/fe-core/src/test/java/org/apache/doris/cloud/transaction/CloudCommittedTsoTest.java#L95-L135). - Tracker abandonment removes the exact attempted TSO and signals waiters; the committed-prefix candidate can subsequently advance beyond it. The fencing helper instead waits for an effective fence at least as large as the attempted TSO before abandonment, and deactivates the TSO service if fencing fails. See [tracker](https://github.com/apache/doris/blob/72e91896bbdd8760f8bd4e488e5672406b40ce52/fe/fe-core/src/main/java/org/apache/doris/tso/TSOTransactionTracker.java#L110-L179) and [TSO service](https://github.com/apache/doris/blob/72e91896bbdd8760f8bd4e488e5672406b40ce52/fe/fe-core/src/main/java/org/apache/doris/tso/TSOService.java#L238-L251). - Crucially, the server-side `MetaServiceProxy::call_impl` rewrites `KV_TXN_MAYBE_COMMITTED` to `KV_TXN_COMMIT_ERR` when retries are disabled, or retries it and performs that conversion on exhaustion. Both the normal and compatibility `actual_code` therefore lose the maybe-committed distinction; the existing cloud tests check this. The FE currently also abandons without fencing for `KV_TXN_COMMIT_ERR`. Changing only the named raw-code branch would leave this production path uncovered. See [MS wrapper](https://github.com/apache/doris/blob/72e91896bbdd8760f8bd4e488e5672406b40ce52/cloud/src/meta-service/meta_service.h#L1083-L1148) and [conversion tests](https://github.com/apache/doris/blob/72e91896bbdd8760f8bd4e488e5672406b40ce52/cloud/test/meta_service_test.cpp#L10709-L10834). **Impact boundaries** For bounded, strongly consistent cloud `@incr` reads, releasing a registration removes that attempt from the admission wait. If its metadata commit can still become visible later, a window can complete before those rows exist, leaving them outside the next window. This is a credible correctness risk, conditional on proving that ordering; it is not an observed loss in this investigation. See [window admission](https://github.com/apache/doris/blob/72e91896bbdd8760f8bd4e488e5672406b40ce52/fe/fe-core/src/main/java/org/apache/doris/tso/TSOService.java#L355-L391). The underlying FDB error matters. FoundationDB documents that ordinary `commit_unknown_result` (1021) is already resolved when returned: an unsuccessful attempt cannot subsequently commit. Timeout/cancellation errors have weaker guarantees, and its transaction-options documentation also warns about later success after `cluster_version_changed`. Doris explicitly maps a commit-time `cluster_version_changed` to `TXN_MAYBE_COMMITTED`; commit timeouts map to `KV_TXN_COMMIT_ERR`. Consequently, treating every maybe-committed result as proof of a future commit would be incorrect. See [FDB guarantees](https://apple.github.io/foundationdb/developer-guide.html#the-commit-unknown-result-error), [client-version-change caveat](https://apple.github.io/foundationdb/javadoc/com/apple/foundationdb/TransactionOptions.html#setAutomaticIdempotency()), and [Doris commit mapping](https://github.com/apache/doris/blob/72e91896bbdd8760f8bd4e488e5672406b40ce52/cloud/src/meta-store/txn_kv.cpp#L877-L900). Cloud Table Streams require a separate impact demonstration: their read bounds come from MS partition `commit_tso`, and offset reads drain pending lazy transactions before returning those bounds. They do not simply become readable because this FE registration disappeared. The report does not yet establish an offset advancing past a subsequently visible row. See [stream bounds and pending-transaction handling](https://github.com/apache/doris/blob/72e91896bbdd8760f8bd4e488e5672406b40ce52/cloud/src/meta-service/meta_service_table_stream.cpp#L93-L273). **Suggested next steps and missing evidence** 1. Coordinate with #69925, whose diff fences `KV_TXN_COMMIT_ERR`. Decide whether the raw maybe-committed branch should also fence defensively, or document its unreachable-on-normal-MS contract and reconcile the #67820 description. Do not consider a raw-code-only change sufficient. 2. Exercise the actual MS wrapper with store retries both enabled and disabled, plus a genuinely unresolved commit outcome. Assert that fence acknowledgement precedes tracker release; a failed fence disables allocation/read admission; and a still-uncommitted old attempt is rejected or conflicts after the fence. The MS commit reads the fence in the same non-snapshot FDB transaction, so this barrier has a concrete conflict mechanism, provided both fence-check flags are enabled. See [fence check](https://github.com/apache/doris/blob/72e91896bbdd8760f8bd4e488e5672406b40ce52/cloud/src/meta-service/meta_service_txn.cpp#L136-L169). 3. For an impact reproduction, provide FE/MS commit hashes, FDB client/server versions, retry/timeout and fence-check settings, and the exact injection location. Correlate transaction ID and attempted TSO with the numeric FDB error, final wire `code`/`actual_code`, fence value, tracker removal, committed-prefix advancement, and metadata visibility times. Include table DDL/changelog settings and the two consecutive `@incr` windows; for Table Streams, include partition versions and offsets before/after consumption. Distinguish immediate, subtransaction/2PC, and lazy publication paths. Merely fabricating a response demonstrates cleanup behavior, not a late commit or a missed row. Validation performed: source and existing-test inspection, current-master comparison of the relevant blocks, and public PR inspection. No Doris build, test execution, or runtime fault injection was performed; no repository code was modified. Breakwater-GitHub-Analysis-Slot: slot_2cfef8a6f38b -- 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]
