[fix](cloud) fix get_prepare_txn_by_coordinator method - #67761
Conversation
### What problem does this PR solve? Scan running keys and batch-read txn info in groups of 128. Process each page locally with one iterator loop and resume expired pages without losing results or KV read metrics. The dynamic enable_get_prepare_txn_by_coordinator_by_running_key switch defaults to true; false scans txn info directly. Skip undecodable running keys with a warning. Log scanned and matched counts and identify the RPC in scan errors. ### Release note Reduce coordinator cleanup scan cost, recover from KV transaction expiry, and allow switching to direct txn-info scans. ### Check List (For Author) - Test: Cloud ASAN unit-test build and all 5 coordinator tests passed on the development host. Coverage includes both scan modes, single and multiple page expiry, exhausted page retries and proxy restart, filtering boundaries, malformed records, empty scans, read metrics, and matching results across 128-entry batches. Formatting, git diff --check and clang-tidy passed. - Behavior changed: Yes; bounded running-key scans, expiry recovery and configurable scan strategy. - Does this need documentation: No
|
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 updates get_prepare_txn_by_coordinator to avoid scanning all txn_info_key records in one long-lived KV transaction by optionally scanning txn_running_key records and batch-fetching corresponding txn infos, with retry/resume behavior on KV_TXN_TOO_OLD.
Changes:
- Add a running-key scan mode controlled by
enable_get_prepare_txn_by_coordinator_by_running_key, with batched txn-info reads (128/group). - Add extensive tests covering scan-mode behavior, retries on snapshot expiry, and various error/edge cases.
- Adjust MemTxnKv batch-get accounting behavior (counters).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cloud/test/meta_service_test.cpp | Adds new tests validating running-key scan mode, batching, resume-on-expiry, and metrics expectations. |
| cloud/src/meta-store/mem_txn_kv.cpp | Changes how Transaction::batch_get updates internal/kv get counters. |
| cloud/src/meta-service/meta_service_txn.cpp | Implements running-key scan + batch-get logic, plus expiry retry and error handling for malformed/missing txn info. |
| cloud/src/common/config.h | Adds config switch for choosing scan mode (running-key vs info-key). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @@ -815,8 +815,6 @@ TxnErrorCode Transaction::batch_get(std::vector<std::optional<std::string>>* res | |||
| auto ret = inner_get(k, &val, opts.snapshot); | |||
| ret == TxnErrorCode::TXN_OK ? res->push_back(val) : res->push_back(std::nullopt); | |||
| } | |||
| kv_->get_count_ += keys.size(); | |||
There was a problem hiding this comment.
the inner_get already call it
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review result: REQUEST_CHANGES. This review is complete for head 038d77a3eb6732308fa3f874b2df532983d9933a after two required review rounds. One new P1 correctness issue remains; existing inline threads were treated as duplicate fences and were not repeated.
Part 1.3 checkpoint conclusions
- Goal and proof: The running-index scan and 128-key batch reads address the intended scan-cost problem, and the tests cover modes, filters, pagination, error propagation, retry exhaustion, config capture, and metrics. The page-local expiry recovery does not preserve a coherent final view, however, so the overall goal is not safely complete. The missing proof is an earlier-page
PREPAREDtoPRECOMMITTEDtransition before a later-pageTXN_TOO_OLD. - Scope and focus: The four changed files are focused on the scan/retry change, its mutable fallback, MemTxnKv accounting, and tests. No unrelated change was identified.
- Concurrency and locking: The relevant concurrency is between asynchronous FE heartbeat cleanup and an in-flight BE-coordinated 2PC precommit. Each FDB transaction is isolated and lifecycle writes are atomic, but no lock or snapshot spans the multiple read versions combined into one response; that gap is the inline P1. No new lock-order or deadlock issue was found.
- Lifecycle: Creation, precommit, immediate/lazy commit, abort, and recycler paths were traced. Healthy
PREPAREDtransactions have running keys, and terminal transitions remove them consistently. No new static-initialization, ownership-cycle, or shutdown-lifecycle issue was found. - Configuration: The new
CONF_mBoolis dynamically mutable and captured once per RPC, so a current RPC stays on one key family and later RPCs observe changes. The fallback info-key path remains available. - Compatibility: No protobuf, persisted-key encoding, function-symbol, FE/BE variable, or EditLog contract changes are introduced. The running index is pre-existing, so no rolling-upgrade incompatibility was found.
- Parallel paths and conditions: Both scan modes share the same filter and retry branch. Failed range or batch reads append no current-page result, and exclusive continuation avoids duplicates, but prior-page results remain stale after renewal. Other malformed-key and spelling concerns are already covered by existing inline threads.
- Test coverage: The added unit tests are broad for the exercised MemTxnKv paths, but they never mutate a transaction already appended by an earlier page before renewing on a later page. A regression test for that sequence is required. No FDB partial-result correctness leak was found because errors are checked before processing.
- Test results: No build or test was run in this review runner because the review task explicitly prohibits it. Visible CI passed formatting, CheckStyle, license, and secret checks; no functional cloud build/test result was available. No generated result file is changed.
- Observability: Scan/match counts, mode logging, error context, and detailed KV counters are adequate for this path. The FDB failed-attempt counter nuance is telemetry-only and not a control-flow dependency.
- Transactions and persistence: The handler itself is read-only and adds no persistent format. Lifecycle index/info updates remain atomic. The blocker is downstream transaction correctness: a stale returned ID can make cleanup abort a durable
PRECOMMITTED2PC transaction. - Data writes and crash behavior: No new write sequence is added by the PR, and no separate crash leak or master-failover issue was found. The existing abort-by-ID path is the destructive consumer that gives the stale-read bug its impact.
- FE/BE variables: No new cross-process session variable or scattered thrift propagation is involved.
- Performance and memory: Selecting running keys and bounding info batches to 128 removes the broad historical scan without a substantiated transaction-size or allocation regression. Response growth is pre-existing.
- Other issues and user focus: No additional user focus was provided. After the final full-diff sweep and live-thread audit, no other distinct issue remains; the MemTxnKv removal is correct because
inner_getalready performs per-key accounting.
|
|
||
| if (!it->has_next()) { | ||
| begin_info_key = k; | ||
| do { |
There was a problem hiding this comment.
[P1] Revalidate matches after changing the read version
Once a later page returns TXN_TOO_OLD, this creates a new snapshot but leaves txn_infos already appended from earlier pages. For example, page 1 can append A as PREPARED, then precommit_txn changes A to PRECOMMITTED while retaining its running key; this retry resumes past A, so the successful response still contains stale A. FE discards each returned transaction's status and aborts by db/txn ID, and the abort path permits PRECOMMITTED, so coordinator cleanup can roll back a live 2PC transaction that this RPC's PREPARED filter is meant to exclude. Before this change, the handler returned TXN_TOO_OLD; the default outer proxy retry cleared and rescanned the whole response. Please revalidate accumulated matches in the renewed view or restart/clear the scan, and cover an earlier-page PREPARED-to-PRECOMMITTED transition in the retry test.
| : txn_info_key({instance_id, 0, 0}); | ||
| std::string end_key = scan_by_running_key ? txn_running_key({instance_id, INT64_MAX, INT64_MAX}) | ||
| : txn_info_key({instance_id, INT64_MAX, INT64_MAX}); | ||
| LOG(INFO) << "begin_key:" << hex(begin_key) << " end_key:" << hex(end_key); |
There was a problem hiding this comment.
add log scan_by_running_key？
| if (!scan_by_running_key) { | ||
| return TxnErrorCode::TXN_OK; | ||
| } | ||
| std::vector<std::optional<std::string>> info_values; |
There was a problem hiding this comment.
Added reserve() for info_keys. batch_get() already calls reserve() internally for info_values.
### What problem does this PR solve? Problem Summary: Include the scan mode in the initial coordinator cleanup log so failed scans also identify their strategy. Reserve txn-info key capacity using the current running-key page size to avoid vector growth. ### Release note None ### Check List (For Author) - Test: Formatting and git diff --check passed; no build or tests run as requested. - Behavior changed: No; logging and allocation only. - Does this need documentation: No
| code = cast_as<ErrCategory::READ>(err); | ||
| ss << "failed to get txn info. err=" << err; | ||
| msg = ss.str(); | ||
| auto process_txn_info = [&](std::string_view key, std::string_view value) -> TxnErrorCode { |
There was a problem hiding this comment.
Both lambdas capture and modify quite a bit of external state by reference. In particular, read_page reports errors through both its return value and the captured code/msg, making the retry path harder to follow. Could we extract the transaction-info processing into a separate function and simplify how page reads manage state and propagate errors?
|
run buildall |
get_prepare_txn_by_coordinatorscans alltxn_info_keyrecords in a single KV transaction, which may frequently fail withKV_TXN_TOO_OLDwhen there are many records. Creating a new transaction and resuming from the failed page addresses the expiry issue, but scanning all transaction info records remains expensive.Scan
txn_running_keyrecords instead and batch-read the corresponding transaction info in groups of 128, while retaining the ability to resume from the failed page after transaction expiry.