github-actions[bot] commented on code in PR #67813:
URL: https://github.com/apache/doris/pull/67813#discussion_r3985802176
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/transaction/CloudGlobalTransactionMgr.java:
##########
@@ -491,6 +491,9 @@ private boolean checkTransactionStateBeforeCommit(long
dbId, long transactionId)
+ "] is already aborted. abort reason: " +
transactionState.getReason());
} else if (transactionState.getTransactionStatus() ==
TransactionStatus.COMMITTED
|| transactionState.getTransactionStatus() ==
TransactionStatus.VISIBLE) {
+ if (transactionState.getTransactionStatus() ==
TransactionStatus.VISIBLE) {
Review Comment:
[P1] Do not complete a lazy retry while it is only COMMITTED
A timed-out lazy commit can durably stop at `COMMITTED` while
`TxnLazyCommitTask` is still responsible for publishing the partition versions.
This branch now treats that state as completed, but refreshes only `VISIBLE`;
its callers return success before `executeCommitTxnRequest`,
`afterCommitTxnResp`, or success callbacks run. The lazy task later changes the
transaction to `VISIBLE` without notifying FE, and the periodic syncer is not a
safe fallback because it can be disabled or race the pending partitions after
the phase-one table-version bump. Please wait/repoll through visibility and
then perform the normal cache/callback completion, or install a durable repair
owner before returning success.
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/transaction/CloudGlobalTransactionMgr.java:
##########
@@ -671,6 +680,39 @@ private Map<Long, List<Long>>
updateVersion(CommitTxnResponse commitTxnResponse)
return tablePartitionMap;
}
+ private void refreshPartitionVersions(long dbId, long txnId, List<Long>
tableIds) throws UserException {
+ Database db = Env.getCurrentInternalCatalog().getDbNullable(dbId);
+ if (db == null) {
+ // The database may have been dropped after the original commit.
+ return;
+ }
+ List<CloudPartition> partitions = new ArrayList<>();
+ for (long tableId : Sets.newHashSet(tableIds)) {
+ Table table = db.getTableNullable(tableId);
+ if (table == null || !table.isManagedTable()) {
+ continue;
+ }
+ table.readLock();
+ try {
+ for (Partition partition : ((OlapTable)
table).getAllPartitions()) {
+ partitions.add((CloudPartition) partition);
+ }
+ } finally {
+ table.readUnlock();
+ }
+ }
+ try {
+ for (List<CloudPartition> batch : Lists.partition(partitions,
Config.cloud_get_version_task_batch_size)) {
Review Comment:
[P1] Install the refreshed snapshot under table version locks
`getSnapshotVisibleVersionFromMs` writes these `CloudPartition` caches one
by one, but this new path does not take the affected tables'
`versionWriteLock`s. Cached planners take `versionReadLock`s specifically while
snapshotting partition versions, and both the normal commit path and peer-FE
sync path use the matching write locks. A query can therefore interleave after
P1 is advanced but before P2 and plan one atomically visible transaction as
`[new(P1), old(P2)]`. Please fetch the remote values outside locks, then
install all affected partition values under table version-write locks acquired
in ID order.
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/transaction/CloudGlobalTransactionMgr.java:
##########
@@ -671,6 +680,39 @@ private Map<Long, List<Long>>
updateVersion(CommitTxnResponse commitTxnResponse)
return tablePartitionMap;
}
+ private void refreshPartitionVersions(long dbId, long txnId, List<Long>
tableIds) throws UserException {
+ Database db = Env.getCurrentInternalCatalog().getDbNullable(dbId);
+ if (db == null) {
+ // The database may have been dropped after the original commit.
+ return;
+ }
+ List<CloudPartition> partitions = new ArrayList<>();
+ for (long tableId : Sets.newHashSet(tableIds)) {
+ Table table = db.getTableNullable(tableId);
+ if (table == null || !table.isManagedTable()) {
+ continue;
+ }
+ table.readLock();
+ try {
+ for (Partition partition : ((OlapTable)
table).getAllPartitions()) {
+ partitions.add((CloudPartition) partition);
+ }
+ } finally {
+ table.readUnlock();
+ }
+ }
+ try {
+ for (List<CloudPartition> batch : Lists.partition(partitions,
Config.cloud_get_version_task_batch_size)) {
+ CloudPartition.getSnapshotVisibleVersionFromMs(batch, false);
Review Comment:
[P1] Refresh the table cache and peer FEs too
This fallback only mutates the master FE's `CloudPartition` objects. Unlike
the normal path above, it neither advances each `OlapTable.cachedTableVersion`
nor calls `CloudFEVersionSynchronizer.pushVersionAsync`. With the default
`Long.MAX_VALUE` table/partition cache TTLs, a warmed SQL cache can still
validate against the old table version on the master, and follower/observer FEs
can keep planning with their old partition versions until the periodic daemon
runs. Thus an acknowledged visible retry can still serve stale results. Please
recover the table versions and propagate the recovered partition/table state
through the same synchronizer path as a normal commit response.
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/transaction/CloudGlobalTransactionMgr.java:
##########
@@ -815,8 +857,15 @@ private void executeCommitTxnRequest(CommitTxnRequest
commitTxnRequest, long tra
StopWatch stopWatch = new StopWatch();
stopWatch.start();
try {
- txnState = commitTxn(commitTxnRequest, transactionId, is2PC,
tabletCommitInfos, tabletIds);
+ CommitTxnResponse response = commitTxn(commitTxnRequest,
transactionId, is2PC);
Review Comment:
[P1] Recover already-visible final 2PC retries
For a final 2PC retry, Meta Service returns `TXN_ALREADY_VISIBLE` plus only
`txn_info`, but `commitTxn(..., is2PC=true)` rejects that status before it can
return this response to `afterCommitTxnResp`. The apparent MoW precheck does
not cover this: `commitTransaction2PC` calls `getMowTableList(tableList,
null)`, which always returns an empty list. Thus losing the first final-commit
response leaves every retry failing with stale caches. Please treat the
already-visible final-2PC response as the durable idempotent outcome and run
the same cache recovery before completing it.
##########
fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.java:
##########
@@ -5684,6 +5684,9 @@ public TStatus
reportCommitTxnResult(TReportCommitTxnResultRequest request) thro
// compaction notify update tablet stats
CloudTabletStatMgr.getInstance().addActiveTablets(tabletIds);
}
+ } catch (UserException e) {
+ LOG.warn("failed to refresh versions for reported commit, txnId:
{}", request.getTxnId(), e);
+ return new
TStatus(TStatusCode.INTERNAL_ERROR).setErrorMsgs(Collections.singletonList(e.getMessage()));
Review Comment:
[P1] Give this failed refresh a retry owner
The only production caller of this report is `send_stats_to_fe_async` in
`be/src/cloud/cloud_meta_mgr.cpp`; it converts this `INTERNAL_ERROR` to a
failed status, logs it, and then deliberately returns `Status::OK()` without
retrying. If an already-visible response reaches FE while `VersionHelper`
exhausts its bounded retries, BE has already acknowledged the commit and this
cache repair is never replayed, so the old version can remain cached after Meta
Service recovers. Please retry/schedule the repair in FE or make the BE report
path retry non-OK results; returning an error that its sole caller drops does
not close the stale-read path.
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/transaction/CloudGlobalTransactionMgr.java:
##########
@@ -815,8 +857,15 @@ private void executeCommitTxnRequest(CommitTxnRequest
commitTxnRequest, long tra
StopWatch stopWatch = new StopWatch();
stopWatch.start();
try {
- txnState = commitTxn(commitTxnRequest, transactionId, is2PC,
tabletCommitInfos, tabletIds);
+ CommitTxnResponse response = commitTxn(commitTxnRequest,
transactionId, is2PC);
+ txnState = TxnUtil.transactionStateFromPb(response.getTxnInfo());
+ // A cache refresh failure must not make callbacks treat a durable
commit as failed.
txnOperated = true;
+ if (MetricRepo.isInit) {
+ MetricRepo.COUNTER_TXN_SUCCESS.increase(1L);
+
MetricRepo.HISTO_TXN_EXEC_LATENCY.update(txnState.getCommitTime() -
txnState.getPrepareTime());
+ }
+ afterCommitTxnResp(response, tabletCommitInfos, tabletIds);
Review Comment:
[P1] Do not return a normal commit failure after success callbacks
At this point Meta Service has returned the durable visible transaction and
`txnOperated` is already true, so `finally` runs
`afterCommitted`/`afterVisible`; nevertheless a refresh exception escapes from
this call. `TransactionEntry.commitTransaction` interprets that as a failed
commit, attempts to abort the already-visible transaction, and reports failure.
Routine Load is worse: `afterVisible` renews/removes the old txn task, so
retrying the returned error fails in `beforeCommitted` before cache repair can
run. Please make repair independent/retryable (or invalidate caches) while
returning an outcome that callers cannot route to abort or replay callbacks.
--
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]
