[fix](cloud) fix get_prepare_txn_by_coordinator method by mymeiyi · Pull Request #67761 · apache/doris

mymeiyi · 2026-09-10T03:48:26Z

get_prepare_txn_by_coordinator scans all txn_info_key records in a single KV transaction, which may frequently fail with KV_TXN_TOO_OLD when 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_key records 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.

### 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

hello-stephen · 2026-09-10T03:48:31Z

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

What problem was fixed (it's best to include specific error reporting information). How it was fixed.
Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
What features were added. Why was this function added?
Which code was refactored and why was this part of the code refactored?
Which functions were optimized and what is the difference before and after the optimization?

mymeiyi · 2026-09-10T03:57:54Z

/review

Copilot

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.

gavinchou · 2026-09-10T04:22:18Z

@@ -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();


why to remove these?

the inner_get already call it

github-actions

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 PREPARED to PRECOMMITTED transition before a later-page TXN_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 PREPARED transactions 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_mBool is 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 PRECOMMITTED 2PC 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_get already performs per-key accounting.

github-actions · 2026-09-10T04:58:42Z


-            if (!it->has_next()) {
-                begin_info_key = k;
+    do {


[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.

deardeng · 2026-09-10T07:02:32Z

+                                                : 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);


add log scan_by_running_key？

gavinchou · 2026-09-10T08:00:25Z

+        if (!scan_by_running_key) {
+            return TxnErrorCode::TXN_OK;
+        }
+        std::vector<std::optional<std::string>> info_values;


reserve first?

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

deardeng · 2026-09-10T08:46:01Z

-            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 {


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?

mymeiyi · 2026-09-11T06:09:29Z

run buildall

mymeiyi · 2026-09-16T06:44:43Z

/review

github-actions

Review result: REQUEST_CHANGES for head 52dbed63a04aa52fb3276f5fd45d83df98dc69cc.

I found one new P2 inline issue: the new page-local TXN_TOO_OLD retry can repeat and conceal a production FDB range-future leak. The existing P1 mixed-snapshot finding also remains applicable on this head and was not duplicated.

Review checkpoint conclusions

Goal, proof, scope, and clarity: The running-key scan plus 128-key info batches addresses the intended scan-cost problem, preserves the old coordinator/status/start-time predicates, and retains a mutable direct-info fallback. The four-file change is focused. The lambda-structure concern is already covered by an existing thread and produced no separate correctness finding.
Correctness, concurrency, and lifecycle: The handler is RPC-local and synchronous, with no new locks or lock-order concern. I traced running-index creation and removal through begin, precommit, immediate/eventual/lazy commit, abort, recycler, 2PC, and parent/subtransaction paths; those writes remain atomic with transaction-info state. Page-local snapshot renewal can retain stale earlier-page matches, but that destructive PREPARED to PRECOMMITTED case is the existing P1 linked above. No additional skip, duplicate, continuation, or proxy-response-clearing defect survived review.
Resource ownership and error paths: Successful range iterators and batch reads have owners, but production FDB range errors can return before RangeGetIterator takes ownership of the raw future. The changed local retry amplifies that pre-existing helper leak and can hide it behind a successful RPC; this is the new inline P2. The MemTxnKv retry tests do not exercise production FDB future lifetime.
Configuration and compatibility: The mutable mode flag is sampled once per RPC, so live changes affect a later RPC without mixing modes during retry. There is no wire, persisted-value, or key-format change; the running-key format predates this PR, and the direct scan remains available for rollback. No FE/BE variable propagation is involved.
Conditions and malformed data: Missing info and malformed info fail explicitly. Unsafe malformed running-key decoding is already covered by an existing thread. I found no supported writer, migration, repair, mixed-version, or ambiguous-commit path that creates a valid prepared transaction whose only index key is malformed, so I did not add a second omission comment.
Transactions, persistence, failover, and observability: The changed production handler is read-only and adds no EditLog or storage mutation. Existing transaction/index transitions remain atomic. Scan mode/count logging and accumulated KV metrics are adequate; the MemTxnKv counter deletion fixes double counting because inner_get already accounts for each point read.
Performance and limits: Running-key pages and point-read fanout are bounded at 128 and avoid historical transaction-info reads. The response was already unbounded by the API's result semantics. Apart from the accepted future leak, no additional CPU, copy, transaction-size, or memory-growth issue was substantiated.
Tests and results: The added tests cover both modes, filtering, paging, local and whole-RPC retry behavior, response clearing, malformed/missing records, dynamic configuration capture, metrics, and large values. Missing coverage remains for the existing earlier-page state-transition P1 and for the new real-FDB error-lifetime P2. This review was static-only as required; I did not run builds or tests. On the exact live head, formatter, Checkstyle, compile, Cloud UT, P0/non-concurrent/cloud/vault/external regressions report success; FE/BE UT and coverage report successful skip statuses. The automated review status itself remains pending until this review completes.
User focus and final sweep: No additional user focus was supplied. Two bounded rounds converged, the complete authoritative diff was reread, and every candidate is now submitted, duplicate-fenced, or dismissed with concrete evidence; no unresolved review candidate remains.

github-actions · 2026-09-16T07:18:58Z

-                begin_info_key = k;
+    do {
+        err = read_page();
+        if (err == TxnErrorCode::TXN_TOO_OLD) {


[P2] Destroy failed range futures before retrying

A real production FDB range TXN_TOO_OLD returns from Transaction::get(begin, end, ...) before its raw FDBFuture* is transferred to RangeGetIterator; txn.reset() here destroys only the FDBTransaction, not that future. The helper's raw-pointer bug predates this PR, but this new local retry can repeat it once per expired page (or twice when the retry also fails) and then return OK while retaining those futures. The MemTxnKv callbacks inject after a successful owned read and cannot exercise this lifetime path. Please give the range future immediate RAII ownership, release it only when transferring it to RangeGetIterator, and add an FDB-backed error-path ownership test.

deardeng

LGTM

/dimiss

github-actions · 2026-09-16T07:51:39Z

PR approved by at least one committer and no changes requested.

`get_prepare_txn_by_coordinator` scans all `txn_info_key` records in a single KV transaction, which may frequently fail with `KV_TXN_TOO_OLD` when 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_key` records 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.

…on PRs from master in merge order (#67783 #68034 #68033 #68057 #68094 #68087) (#68151) Cherry-picked from #67783, #68034, #68033, #68057, #68094, #68087 Batch pick of every merged PR carrying the `incremental-computation` label that `branch-incremental-computation` does not have yet (no `incremental-computation-picked` label), in the order they landed on master (`git log --first-parent`). One commit per PR, each created with `git cherry-pick -x` so the message ends with `(cherry picked from commit <master sha>)`. Follows the same convention as #67830, #68017 and #68073. | # | Master commit | PR | Title | |---|---|---|---| | 1 | e545b13 | #67783 | [fix](policy) Enforce row policies on MOW time travel | | 2 | f7a0842 | #68034 | [fix](binlog) Fix missing DELETE events in row binlog | | 3 | e85575e | #68033 | [fix](binlog) Preserve row binlog compaction policy | | 4 | 21160d7 | #68057 | [fix](binlog) Decouple row binlog compaction from CCR binlog config | | 5 | 04aa5a1 | #68094 | [fix](binlog) Persist row binlog config updates in cloud mode | | 6 | da8feed | #68087 | [fix](binlog) Report streams with missing base tables as stale | Not included on purpose: - The 20 labelled PRs that already carry `incremental-computation-picked` (#62606 in the fork point, #67508 via #67712, the nine of #67830, the six of #68017, the two of #68073, and #68050 whose content this branch got directly through #68012). - #68012 carries the label but is a PR against this branch itself (merged as `6f7c87fa892`); nothing to pick. - #67820 is still open on master; this branch already carries its content via #67861. ### Prerequisite check For every pick I listed the master commits between the fork point (`efedf10c7e3`) and the pick that touch the same files and are not on this branch, and checked whether the pick's behavior depends on them. - **#67783** declares no related PR. It builds on the MOW time-travel rewrite (`BindRelation.buildMowTimeTravelUnion`, #67480) which is already here. Two unlabelled master commits overlap: #66770 (authorization plugin SPI: reworks the data-mask / row-filter API of `LogicalCheckPolicy` and its test) and #67811 (removes the `isPlayNereidsDump()` check in `CheckPolicy`). Neither is a functional prerequisite: the fix consists of `getPolicyTable()` unwrapping `OlapTableWrapper`, `CheckPolicy` collecting the whole filter chain below the policy mask, and `BindRelation` putting a `LogicalCheckPolicy` above each union branch — none of that uses the #66770 API. Not picked; adapted instead (see below). - **#68034**, **#68033**, **#68087** (#68087 relates to #67173, which is before the fork point): no unpicked master commit touches any of their files; the picks applied cleanly and are byte-identical to master. - **#68057**: only `be/test/cloud/cloud_compaction_test.cpp` overlaps with the unrelated #67972 (refresh tablet meta of continuously ingested tablets); auto-merged, the pick only adds new `TEST_F` blocks. Main-code hunks are byte-identical to master. - **#68094**: overlaps with #68090 / #67972 / #66598 (`cloud_tablet.cpp`), #67295 / #67618 / #68090 (`base_tablet.{h,cpp}`), #66598 / #67637 (`meta_service.cpp`), #66598 (`cloud.proto`, `cloud_tablet_test.cpp`) and #67761 / #66598 / #67637 (`meta_service_test.cpp`). All auto-merged. The fix — `BaseTablet::binlog_config()` under the meta lock, `BinlogConfig::operator==`, `CloudTablet::sync_meta()` refreshing the binlog config, `update_tablet` in the meta service accepting `binlog_config`, and `CloudSchemaChangeHandler` routing ROW-binlog property updates through it — does not use anything those commits add. Its hunks are byte-identical to master except one trailing context line in `sync_meta()` (`last_sync_tablet_meta_time_s` comes from #67972). Only #67783 needed adaptation, recorded in its commit message: - `LogicalCheckPolicy.java`: the conflicting context was master's `parsePolicyExpression()` helper (#66770), which does not exist here; the new `getPolicyTable()` is inserted in the same place without it. - `CheckRowPolicyTest.java`: the data-mask mock is written against this branch's per-column `AccessControllerManager.evalDataMaskPolicy(..., column) -> Optional<DataMaskPolicy>` instead of master's `evalDataMaskPolicies(..., Set<String>) -> Map<String, DataMaskSpec>`. The masks are the same (concat for the random-distribution table; the non-movable `k2` mask plus identity masks for the hidden reconstruction columns of the MOW table). The master-only `Or` import (#66770) is not carried; `Collections` / `Locale` imports were added because they arrive with #66770 on master. ### Drift check against master Each pick's `+`/`-` lines are identical to the master commit's, except for the #67783 adaptation above. After the six picks, the touched files still differ from master at `da8feed859d` in: `cloud_tablet.cpp`, `base_tablet.{h,cpp}`, `cloud_compaction_test.cpp`, `cloud_tablet_test.cpp`, `meta_service.cpp`, `meta_service_test.cpp`, `cloud.proto`, `CheckPolicy.java`, `LogicalCheckPolicy.java`, `CheckRowPolicyTest.java`. Replaying the nine unpicked master commits listed above (#67637, #67618, #66770, #66598, #67972, #67811, #67761, #67295, #68090) in a temporary index and removing this branch's own #67861 (`GetTsoRecoveryTransactions` in `cloud.proto` / `meta_service_test.cpp`) brings every file to zero diff against master, except the two `LogicalCheckPolicy.java` / `CheckRowPolicyTest.java` hunks of #66770 that overlap the adaptation. Nothing else is left over. ### Verification - FE: `run-fe-ut.sh --run` on this branch (regenerates thrift/protobuf, compiles fe-core main + test) with the test classes touched by the picks plus `DeleteFromCommandTest` from #68034's checklist: 5 classes, 58 tests, 0 failures, 0 errors, BUILD SUCCESS — `CheckRowPolicyTest` 9 (the three tests #67783 adds included), `ExplainTableStreamPlanTest` 24, `CloudSchemaChangeHandlerTest` 18 (the tests of #68033 and #68094 included), `DropTableStreamTest` 5, `DeleteFromCommandTest` 2. - FE checkstyle on fe-core: 0 violations. - BE: `-fsyntax-only` with the flags of the Release build (`compile_commands.json`, regenerated `gen_cpp` headers incl. the new `TabletMetaInfoPB.binlog_config`) passes for `cloud/cloud_tablet.cpp`, `cloud/cloud_storage_engine.cpp`, `storage/olap_server.cpp`, `storage/tablet/tablet.cpp`, `storage/tablet/base_tablet.cpp`, and with `-DBE_TEST -fno-access-control` for `test/cloud/cloud_compaction_test.cpp`, `test/cloud/cloud_tablet_test.cpp`, `test/storage/compaction/compaction_task_test.cpp`, `test/storage/tablet/tablet_test.cpp`. - Meta service: the two-line `update_tablet` hunk of #68094 only uses the generated `TabletMetaInfoPB::has_binlog_config()/binlog_config()` and `TabletMetaCloudPB::mutable_binlog_config()` accessors, all present in the regenerated headers. - The two new groovy suites (`row_binlog_p0/test_row_binlog_mow_light_delete`, `time_travel_p0/test_mow_time_travel_row_policy`) parse cleanly (groovy parser check). --------- Co-authored-by: morrySnow <zhangwenxin@selectdb.com> Co-authored-by: Luwei <814383175@qq.com>

…on PRs from master in merge order (#67820 #68088) (#68236) Cherry-picked from #67820, #68088 Batch pick of every merged PR carrying the `incremental-computation` label that `branch-incremental-computation` does not have yet (no `incremental-computation-picked` label), in the order they landed on master (`git log --first-parent`). One commit per PR, each ending with `(cherry picked from commit <master sha>)`. Follows the same convention as #67830, #68017, #68073 and #68151. | # | Master commit | PR | Title | |---|---|---|---| | 1 | 44e3ae2 | #67820 | [fix](binlog) Track committed TSO and fence uncertain commits for bounded incremental reads | | 2 | 3de3a75 | #68088 | [fix](binlog) Require SELECT privilege for binlog TVF | Not included on purpose: - The 25 labelled PRs that already carry `incremental-computation-picked`. - #68012 carries the label but is a PR against this branch itself (merged as `6f7c87fa892`); nothing to pick. ### How #67820 was picked This branch already carried #67820 through #67861, which backported the PR's first seven commits (up to `17272039558`) before the PR was merged. Between that cut and the merge the PR gained six more commits and five master merges, and the final design differs from what #67861 brought: the Meta Service recovery RPC (`get_tso_recovery_transactions`) and the FE startup/periodic recovery scan are gone, replaced by a durable per-instance commit-TSO fence (`txn_tso_fence_key`, `advance_tso_fence`, commit-time `TXN_COMMIT_TSO_EXPIRED` check, `enable_check_commit_tso_fence`), plus the review-feedback and "release maybe-committed TSO" fixes. A plain `cherry-pick -x 44e3ae2` conflicts in 23 files because the branch holds the intermediate design, so commit 1 was built by replaying, on top of the branch, exactly what the PR gained after the cut: - the PR's later commits `d16e67197b2`, `e53291d196b`, `864546d5531`, `8c55eb086e9`, `8d264ce294a`, `fc4f9969202`; - the content that landed inside its master merges, identified by diffing each merge against its `git merge-tree` automerge result: `c491a293426` (`setEnvTSOService` replaced by `Mockito.doReturn(tsoService).when(masterEnv).getTSOService()` — `masterEnv` is the same Mockito delegating mock here since #67813) and `29f133584b5` (the extra `TSOTransactionTrackerTest` coverage and two comment removals in `DorisFlightSqlProducer`). The other three merges only resolved import blocks against master-only code. The replay then got squashed into one commit with the master PR's message, the original author, and the `(cherry picked from commit 44e3ae2)` trailer. Adaptations, all recorded in the commit message: - `DorisFlightSqlProducer` / `DorisFlightSqlProducerTest` stay under `service/arrowflight` (the branch lacks the package move of #67866). - `8d264ce294a` is a no-op here: it removes a `catch (FlightRuntimeException)` block that #67883 added on master and this branch never had. - The branch-only `get_tso_recovery_transactions` RPC, its recovery scan, the five `TsoRecovery*` Meta Service tests and the NOLINT suppressions #67861 had added for them are removed, as on master. - The round-3 `mockVersionHelper()` adaptation in `CloudGlobalTransactionMgrTest` (no `VersionHelper.getVersionFromMeta(req, maxAttempts)` overload here, #66296) is kept. ### Prerequisite check - **#67820** declares #67181 and #67594 as related; #67181 (`e5a4e725fac`) is before the fork point and #67594 came with #67830. The rest of what the commit touches on master is import-block and neighbouring-code drift from unlabelled commits (#67866 / #67883 / #67966 Arrow Flight and session refactors, #67761 `get_prepare_txn_by_coordinator`, #66598 pre-rowset delete bitmaps, repair-tablet-index changes); none of it is used by the fence/committed-TSO logic. - **#68088** declares no related PR. The hook it implements (`TableValuedFunctionIf.checkAuth`) and the caller chain (`CheckPrivileges.visitLogicalTVFRelation` → `TableValuedFunction.checkAuth` → catalog function) are byte-identical between this branch and master, and the five-argument `AccessControllerManager.checkTblPriv` overload exists. Applied cleanly. ### Drift check against master - **#67820**: every one of the 54 files the master commit touches now contains the pick's content — the master commit reverse-applies cleanly per file onto this branch (50 files), and the four files where only the surrounding context differs (`config.h`, `meta_service_txn.cpp`, `StmtExecutor.java`, `StmtExecutorTest.java`) contain every added line and none of the removed ones. 33 of the 54 files are byte-identical to master at `44e3ae2b951`, including all of `fe/.../tso/`, `CloudGlobalTransactionMgr.java`, `MetaServiceProxy.java`, `keys.{h,cpp}`, `meta_service.h` and the regression suite/output. The remaining differences are unrelated master-only or branch-only code from unlabelled commits (#67761, #66598, #67866/#67883/#67966, repair-tablet-index, meta-cache columns, recycler configs) plus the documented `mockVersionHelper()` adaptation; no line in the committed-TSO/fence domain is left over from #67861. - **#68088**: all three files are byte-identical to master at `3de3a756f74`. ### Verification - FE: `run-fe-ut.sh --run` on this branch (regenerates thrift/protobuf, compiles fe-core main + test) with every test class the picks touch: 12 classes, 175 tests, 0 failures, 0 errors, BUILD SUCCESS — `TSOServiceTest` 38, `CloudGlobalTransactionMgrTest` 37, `StmtExecutorTest` 27, `MetaServiceProxyTest` 22, `OlapScanNodeTest` 12, `TimeBasedChangeVisibleWaiterTest` 12, `TSOTransactionTrackerTest` 9 (the four tests added inside the PR's last master merge included), `DorisFlightSqlProducerTest` 7, `TsoStatusMetadataGeneratorTest` 5, `CloudCommittedTsoTest` 3, `SchemaTableTest` 2, `TableBinlogFunctionAuthTest` 1 (#68088). The `@Test` counts of the touched classes equal master's. - FE checkstyle on fe-core: 0 violations. - Meta Service: `-fsyntax-only` with the flags of `cloud/CMakeLists.txt` (`-Wall -Werror`, regenerated `gen_cpp/cloud.pb.h` with `AdvanceTsoFence*` / `TxnTsoFencePB` and without `GetTsoRecoveryTransactions*`) on `meta_service_txn.cpp`, `keys.cpp`, `http_encode_key.cpp`, `bvars.cpp` and, with `-DUNIT_TEST -DBE_TEST -fno-access-control`, on `meta_service_test.cpp`, `keys_test.cpp`, `http_encode_key_test.cpp`, `meta_service_helper_test.cpp`, `txn_lazy_commit_test.cpp`: no diagnostic in any line the pick touches (the only errors are the pre-existing macOS-only `pthread_setname_np` / `int64_t`-vs-`long` ones in untouched 2024/2025 code). clang-format 16 is clean on all 14 touched cloud files. - No BE file changes in this round (the BE side of #67820 was already byte-identical to master via #67861). - The new `auth_p0/test_binlog_tvf_auth.groovy` and the two #67820 suites parse cleanly (groovy parser check). --------- Co-authored-by: Luwei <814383175@qq.com> Co-authored-by: morrySnow <zhangwenxin@selectdb.com>

Copilot AI lite review requested due to automatic review settings September 10, 2026 03:48

mymeiyi requested review from gavinchou, liaoxin01 and luwei16 as code owners September 10, 2026 03:48

Copilot AI reviewed Sep 10, 2026

View reviewed changes

Comment thread cloud/src/meta-service/meta_service_txn.cpp

Comment thread cloud/src/meta-store/mem_txn_kv.cpp

Comment thread cloud/src/meta-service/meta_service_txn.cpp

Copilot started reviewing on behalf of mymeiyi September 10, 2026 04:10 View session

gavinchou reviewed Sep 10, 2026

View reviewed changes

github-actions Bot requested changes Sep 10, 2026

View reviewed changes

gavinchou added dev/3.1.x dev/4.1.x labels Sep 10, 2026

gavinchou assigned deardeng and luwei16 Sep 10, 2026

deardeng reviewed Sep 10, 2026

View reviewed changes

gavinchou reviewed Sep 10, 2026

View reviewed changes

deardeng reviewed Sep 10, 2026

View reviewed changes

github-actions Bot previously requested changes Sep 16, 2026

View reviewed changes

deardeng approved these changes Sep 16, 2026

View reviewed changes

gavinchou approved these changes Sep 16, 2026

View reviewed changes

mymeiyi merged commit 37c97fe into apache:master Sep 16, 2026
39 checks passed

github-actions Bot added the approved Indicates a PR has been approved by one committer. label Sep 16, 2026

github-actions Bot mentioned this pull request Sep 16, 2026

branch-4.1: [fix](cloud) fix get_prepare_txn_by_coordinator method #67761 #68059

Open

morningman mentioned this pull request Sep 18, 2026

branch-incremental-computation: pick the merged incremental-computation PRs from master in merge order (#67783 #68034 #68033 #68057 #68094 #68087) #68151

Merged

morningman mentioned this pull request Sep 20, 2026

branch-incremental-computation: pick the merged incremental-computation PRs from master in merge order (#67820 #68088) #68236

Merged

Conversation

mymeiyi commented Sep 10, 2026

Uh oh!

hello-stephen commented Sep 10, 2026

Uh oh!

mymeiyi commented Sep 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gavinchou Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

mymeiyi Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Part 1.3 checkpoint conclusions

Uh oh!

github-actions Bot Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

deardeng Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

mymeiyi Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

gavinchou Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

mymeiyi Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

deardeng Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

mymeiyi commented Sep 11, 2026

Uh oh!

mymeiyi commented Sep 16, 2026

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Review checkpoint conclusions

Uh oh!

github-actions Bot Sep 16, 2026

Choose a reason for hiding this comment

Uh oh!

deardeng left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Sep 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants