yujun777 commented on code in PR #68662:
URL: https://github.com/apache/doris/pull/68662#discussion_r4142375683
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/InsertOverwriteTableCommand.java:
##########
@@ -386,12 +440,87 @@ private static void
failBetweenTheTwoHalvesOfAnOverwrite(TableIf targetTable) th
throw new UserException("debug point: " +
DEBUG_POINT_FAIL_BETWEEN_THE_HALVES_OF_AN_OVERWRITE);
}
+ /**
+ * Cancels this overwrite when the debug point names the table it targets;
see the constants above.
+ * Nothing here decides what a cancelled overwrite means -- the call sites
do, and they differ: the ones
+ * before the rows are durable take the statement back, the one after them
does not.
+ *
+ * <p>One lookup, because a point is consumed by the lookup that reads it:
reading it twice with
+ * {@code execute=1} armed would have the first read spend the allowance
and the point be gone before the
+ * second, which would silently leave the overwrite uncancelled.
+ */
+ private void cancelTheOverwriteAt(String debugPointName, TableIf
targetTable) {
+ if
(!targetTable.getName().equals(DebugPointUtil.getDebugParamOrDefault(
+ debugPointName, "table_name", ""))) {
+ return;
+ }
+ LOG.info("debug point {} cancels the overwrite of {}", debugPointName,
targetTable.getName());
+ cancel();
+ }
+
+ /**
+ * Publishes this overwrite by swapping the temp partitions in, with a
last look at the cancellation flag
+ * taken under the lock the swap contends for.
+ *
+ * <p>{@link #run} reads the flag before the swap is issued, and the swap
then waits for the table's write
+ * lock, so a cancellation that arrives during that wait is the one place
a check before the swap cannot
+ * see. Reading it again here costs nothing and is where the wait happens:
for a cancellation with nothing
+ * committed there is nothing durable to publish, so refusing to swap
costs the statement and leaves the
+ * rows the client asked to keep -- while a swap that went ahead would
replace them with empty partitions.
+ *
+ * <p>Only a local table is wrapped: a remote table swaps on the frontend
that owns it, where this lock
+ * says nothing.
+ */
+ private void publishTheOverwrite(TableIf targetTable, List<String>
partitionNames,
+ List<String> tempPartitionNames, InsertCommandContext insertCtx,
ConnectContext ctx) throws UserException {
+ if (!(targetTable instanceof OlapTable) || targetTable instanceof
RemoteOlapTable) {
+ InsertOverwriteUtil.replacePartition(targetTable, partitionNames,
tempPartitionNames,
+ isForceDropPartition());
+ return;
+ }
Review Comment:
Fixed. The wrapper's `writeLockIfExist()` early return skipped the swap
while `run` went on to `taskSuccess`, and that was a change from what the
utility did: for a dropped table the utility returned from inside a `try` whose
`finally { writeUnlock(); }` then unlocked a lock that return never took, so
the call raised and the statement failed. `publishTheOverwrite` now raises
deliberately, with a message naming the table, so the outcome is the one a
client whose swap never happened is owed -- the catch drops the temp partitions
of the dropped table and takes the task back.
One note on the other route, which I left alone rather than widen this PR:
the auto-detect swap goes through `InsertOverwriteManager.taskGroupSuccess`
into the same utility, so a target dropped under it fails the statement the
same way, with that same unlock-after-early-return shape.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/InsertOverwriteTableCommand.java:
##########
@@ -386,12 +440,87 @@ private static void
failBetweenTheTwoHalvesOfAnOverwrite(TableIf targetTable) th
throw new UserException("debug point: " +
DEBUG_POINT_FAIL_BETWEEN_THE_HALVES_OF_AN_OVERWRITE);
}
+ /**
+ * Cancels this overwrite when the debug point names the table it targets;
see the constants above.
+ * Nothing here decides what a cancelled overwrite means -- the call sites
do, and they differ: the ones
+ * before the rows are durable take the statement back, the one after them
does not.
+ *
+ * <p>One lookup, because a point is consumed by the lookup that reads it:
reading it twice with
+ * {@code execute=1} armed would have the first read spend the allowance
and the point be gone before the
+ * second, which would silently leave the overwrite uncancelled.
+ */
+ private void cancelTheOverwriteAt(String debugPointName, TableIf
targetTable) {
+ if
(!targetTable.getName().equals(DebugPointUtil.getDebugParamOrDefault(
+ debugPointName, "table_name", ""))) {
+ return;
+ }
+ LOG.info("debug point {} cancels the overwrite of {}", debugPointName,
targetTable.getName());
+ cancel();
+ }
+
+ /**
+ * Publishes this overwrite by swapping the temp partitions in, with a
last look at the cancellation flag
+ * taken under the lock the swap contends for.
+ *
+ * <p>{@link #run} reads the flag before the swap is issued, and the swap
then waits for the table's write
+ * lock, so a cancellation that arrives during that wait is the one place
a check before the swap cannot
+ * see. Reading it again here costs nothing and is where the wait happens:
for a cancellation with nothing
+ * committed there is nothing durable to publish, so refusing to swap
costs the statement and leaves the
+ * rows the client asked to keep -- while a swap that went ahead would
replace them with empty partitions.
+ *
+ * <p>Only a local table is wrapped: a remote table swaps on the frontend
that owns it, where this lock
+ * says nothing.
+ */
+ private void publishTheOverwrite(TableIf targetTable, List<String>
partitionNames,
Review Comment:
Real gap, but the decision it asks for is the owning frontend's, so I have
not taken it here.
The cancellation lives on this FE (the client's session), and
`replacePartitions` carries no cancellation state, so the local last-chance
check added in this PR cannot see a KILL that arrives while the owner waits for
its table write lock in `replacePartitionsImpl` -- the check on this side runs
before the RPC, and the wait happens after it. Covering it means either
conveying the verdict (cancelled with nothing committed) in the replacement RPC
so the owner refuses under its own lock, or moving the whole decision to the
owner; both are remote-Doris protocol changes that belong with that feature
rather than with the local overwrite outcome handling this PR is about.
The local half of the same window is covered (`publishTheOverwrite` reads
the flag under the lock), and the auto-detect route is not reachable with
nothing committed on this side, as discussed in the earlier thread.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/RemoteOlapInsertExecutor.java:
##########
@@ -208,6 +208,7 @@ protected void onComplete() throws UserException {
} else {
txnStatus = TransactionStatus.COMMITTED;
}
+ markCommitted();
LOG.info("commit remote txn success, catalog={}, dbId={},
txnId={}, status={}",
Review Comment:
Agreed that the state is possible, and agreed that the caller cannot tell it
apart from an aborted commit: after `masterCallWithRetry` loses the reply, this
FE has no way to know whether the owner committed, and `onFail`'s abort cannot
take a committed remote transaction back.
I would not fix it inside this PR, for the reason the comment itself states
as an alternative: "keeping the partitions while the result is unknown" is not
a stable end state either. The temp partitions it would keep are unpublished,
they belong to a task the overwrite manager still holds, and the next
`allTaskFail` (a master transfer, a restart) drops them -- so the loss is
deferred rather than prevented, with an extra uncertain state in between. What
prevents it is the property this PR's scope note names: the publication being a
committed action of the insert transaction (or the consumption position moving
with the publication), so that "committed" and "published" cannot disagree.
That is a change to the transaction and journal path, not to this command.
Both remote findings are worth tracking; I would file them as their own
issue with the reproduction (a lost commit reply / a KILL during the owner's
swap lock wait) rather than growing this PR.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/OlapInsertExecutor.java:
##########
@@ -240,10 +240,14 @@ protected void onComplete() throws UserException {
ctx.getSessionVariable().getInsertVisibleTimeoutMs(),
txnCommitAttachment,
streamUpdateInfos)) {
txnStatus = TransactionStatus.VISIBLE;
+ markCommitted();
} else {
// Keep the committed status so load accounting and insert result
bookkeeping stay aligned.
txnStatus = TransactionStatus.COMMITTED;
publishTimedOutAfterCommit = true;
+ // Committed, visible later: the rows are durable even though the
session's visibility-timeout
+ // mode may report the timeout as an error. See
InsertCommandContext#setCommitted.
+ markCommitted();
}
Review Comment:
Same family as the remote-commit case above, and it needs the same kind of
decision, which is why I have not taken it here.
Two things make it a cloud transaction-semantics question rather than an
overwrite one. First, the outcome is not decidable locally: after
`KV_TXN_COMMIT_ERR` the meta service may or may not have applied the commit --
`CloudInternalCatalog` already treats that code specially elsewhere -- so
choosing between "publish" and "drop" needs a state query against the meta
service, or a policy for uncertain commits, and it applies to every cloud load
that reaches that path, not only to overwrites. Second, dropping the partitions
on an uncertain commit is wrong in the loss direction and keeping them is wrong
in the stuck direction: an unpublished leftover has no owner that will ever
publish it, since the overwrite that would have is gone. The durable answer is
the same one this PR's scope note names -- make the publication and the commit
one event (or carry the consumption position with the publication) -- and until
that exists, an uncertain commit has no correct local resolution.
Happy to file this alongside the remote findings as a separate issue if that
is useful.
--
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]
