github-actions[bot] commented on code in PR #66987:
URL: https://github.com/apache/doris/pull/66987#discussion_r3827044527
##########
fe/fe-core/src/main/java/org/apache/doris/load/loadv2/BrokerLoadJob.java:
##########
@@ -154,27 +154,33 @@ public void beginTxn()
// before transactionId was assigned (e.g. edit log write
failure), and the pending
// task retry would otherwise burn all retries on this exception
and cancel the job
// with a misleading "Label has already been used".
- Long ownTxnId = findSelfPreparedTxnByLabel();
- if (ownTxnId == null) {
+ TransactionState ownTxn = findSelfTxnByLabel();
Review Comment:
[P1] Recover the visible transaction before source discovery
This lookup is reached only from `BrokerLoadPendingTask.executeTask()` after
`getAllFileStatus()`. In the restart window being fixed, the transaction is
already VISIBLE and no longer depends on the source objects, but a removed
object, expired credential, or storage outage throws before this branch; the
cloud retry path can then journal the job CANCELLED without ever checking its
successful transaction. Please add an owned-transaction recovery probe before
external file discovery (while keeping discovery before creation of a brand-new
txn), and cover a failing listing with an already-VISIBLE txn.
##########
fe/fe-core/src/main/java/org/apache/doris/load/loadv2/BrokerLoadJob.java:
##########
@@ -154,27 +154,33 @@ public void beginTxn()
// before transactionId was assigned (e.g. edit log write
failure), and the pending
// task retry would otherwise burn all retries on this exception
and cancel the job
// with a misleading "Label has already been used".
- Long ownTxnId = findSelfPreparedTxnByLabel();
- if (ownTxnId == null) {
+ TransactionState ownTxn = findSelfTxnByLabel();
+ if (ownTxn == null) {
throw e;
}
- LOG.info("broker load job {} adopts its own prepared txn {} for
label {} on pending task retry",
- id, ownTxnId, label);
- transactionId = ownTxnId;
+ transactionId = ownTxn.getTransactionId();
Review Comment:
[P2] Persist the adopted transaction ID
This assignment is only in memory. The original create-load-job record was
written while `transactionId` was 0, and the `LoadJobFinalOperation` emitted by
`CloudBrokerLoadJob.executeFinish()` does not contain or replay a transaction
ID. If FE restarts again before a new image, replaying the create record plus
the new FINISHED end record leaves SHOW LOAD reporting txn 0. Persist/replay
the recovered ID as part of this finalization and add a create-log ->
recovery-end-log -> second-replay test.
##########
fe/fe-core/src/main/java/org/apache/doris/load/loadv2/BrokerLoadJob.java:
##########
@@ -154,27 +154,33 @@ public void beginTxn()
// before transactionId was assigned (e.g. edit log write
failure), and the pending
// task retry would otherwise burn all retries on this exception
and cancel the job
// with a misleading "Label has already been used".
- Long ownTxnId = findSelfPreparedTxnByLabel();
- if (ownTxnId == null) {
+ TransactionState ownTxn = findSelfTxnByLabel();
+ if (ownTxn == null) {
throw e;
}
- LOG.info("broker load job {} adopts its own prepared txn {} for
label {} on pending task retry",
- id, ownTxnId, label);
- transactionId = ownTxnId;
+ transactionId = ownTxn.getTransactionId();
+ if (ownTxn.getTransactionStatus() == TransactionStatus.VISIBLE) {
+ LOG.info("broker load job {} recovers its own visible txn {}
for label {}",
+ id, transactionId, label);
+ afterVisible(ownTxn, true);
Review Comment:
[P1] Make recovered visibility atomic with cancellation
The pending worker enters this callback without the LoadJob lock.
`LoadJob.afterVisible()` first replays the real commit attachment outside the
lock (restoring LOADING) and only then locks in `updateState(FINISHED)`. A
concurrent `CANCEL LOAD` in that gap can observe LOADING with
`isCommitting=false`, fail to abort the already-VISIBLE txn but still journal
CANCELLED; the recovery update then refuses to replace that final state and
nevertheless audits success. Replay the attachment and choose/log the VISIBLE
terminal state atomically, and add a latch-based cancel-versus-recovery test.
##########
fe/fe-core/src/main/java/org/apache/doris/load/loadv2/BrokerLoadJob.java:
##########
@@ -154,27 +154,33 @@ public void beginTxn()
// before transactionId was assigned (e.g. edit log write
failure), and the pending
// task retry would otherwise burn all retries on this exception
and cancel the job
// with a misleading "Label has already been used".
- Long ownTxnId = findSelfPreparedTxnByLabel();
- if (ownTxnId == null) {
+ TransactionState ownTxn = findSelfTxnByLabel();
+ if (ownTxn == null) {
throw e;
}
- LOG.info("broker load job {} adopts its own prepared txn {} for
label {} on pending task retry",
- id, ownTxnId, label);
- transactionId = ownTxnId;
+ transactionId = ownTxn.getTransactionId();
+ if (ownTxn.getTransactionStatus() == TransactionStatus.VISIBLE) {
+ LOG.info("broker load job {} recovers its own visible txn {}
for label {}",
+ id, transactionId, label);
+ afterVisible(ownTxn, true);
Review Comment:
[P2] Preserve the transaction's durable finish time
This recovery feeds the fetched VISIBLE txn through ordinary
`afterVisible()`, whose `executeFinish()` replaces `finishTimestamp` with
`System.currentTimeMillis()`. The commit attachment was captured before commit,
while `ownTxn.getFinishTime()` contains Meta Service's actual visibility time
(and `replayOnVisible()` deliberately uses that value). After a long outage,
SHOW LOAD duration and `isExpired()` retention are therefore shifted by the
outage length. Finalize and persist the recovered job with the transaction's
durable finish time, and assert it in a delayed-recovery test.
##########
fe/fe-core/src/main/java/org/apache/doris/load/loadv2/BrokerLoadJob.java:
##########
@@ -154,27 +154,33 @@ public void beginTxn()
// before transactionId was assigned (e.g. edit log write
failure), and the pending
// task retry would otherwise burn all retries on this exception
and cancel the job
// with a misleading "Label has already been used".
- Long ownTxnId = findSelfPreparedTxnByLabel();
- if (ownTxnId == null) {
+ TransactionState ownTxn = findSelfTxnByLabel();
+ if (ownTxn == null) {
throw e;
}
- LOG.info("broker load job {} adopts its own prepared txn {} for
label {} on pending task retry",
- id, ownTxnId, label);
- transactionId = ownTxnId;
+ transactionId = ownTxn.getTransactionId();
+ if (ownTxn.getTransactionStatus() == TransactionStatus.VISIBLE) {
+ LOG.info("broker load job {} recovers its own visible txn {}
for label {}",
+ id, transactionId, label);
+ afterVisible(ownTxn, true);
+ } else {
+ LOG.info("broker load job {} adopts its own prepared txn {}
for label {} on pending task retry",
+ id, transactionId, label);
+ }
}
}
- private Long findSelfPreparedTxnByLabel() {
+ private TransactionState findSelfTxnByLabel() {
try {
- Long txnId =
Env.getCurrentGlobalTransactionMgr().getTransactionIdByLabel(dbId, label,
- Lists.newArrayList(TransactionStatus.PREPARE));
+ Long txnId =
Env.getCurrentGlobalTransactionMgr().getTransactionId(dbId, label);
if (txnId == null) {
return null;
}
TransactionState existingTxn =
Env.getCurrentGlobalTransactionMgr().getTransactionState(dbId, txnId);
if (existingTxn != null && existingTxn.getCallbackId() == id
- && existingTxn.getTransactionStatus() ==
TransactionStatus.PREPARE) {
- return txnId;
+ && (existingTxn.getTransactionStatus() ==
TransactionStatus.PREPARE
Review Comment:
[P1] Reconcile an owned COMMITTED transaction instead of cancelling it
Cloud lazy commit is enabled by default, and `commit_txn_eventually()`
durably stores `TXN_STATUS_COMMITTED` before the lazy committer makes it
VISIBLE; the existing failed-lazy-task test also proves that state can persist
when the task is incomplete. After a restart this lookup returns the owned
COMMITTED txn, but this predicate rejects it, so the pending-task failure path
repeatedly attempts an impossible abort and can finally journal CANCELLED even
if the txn later becomes VISIBLE. Treat owned COMMITTED as recovery-in-progress
and wait/poll it to a definitive outcome without restarting loading or
consuming cancellation retries.
--
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]
