yujun777 commented on code in PR #68662: URL: https://github.com/apache/doris/pull/68662#discussion_r4143179178
########## fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/commands/insert/InsertOverwriteDroppedTargetTest.java: ########## @@ -0,0 +1,69 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.nereids.trees.plans.commands.insert; + +import org.apache.doris.catalog.OlapTable; +import org.apache.doris.common.UserException; +import org.apache.doris.common.jmockit.Deencapsulation; +import org.apache.doris.nereids.trees.plans.logical.LogicalPlan; +import org.apache.doris.qe.ConnectContext; + +import com.google.common.collect.Lists; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; +import org.mockito.Mockito; + +import java.util.Optional; + +/** + * What the swap of an overwrite does when its target has been dropped underneath it. + * + * <p>The swap takes the target table's write lock, and a target dropped by a concurrent DROP no longer + * offers one. Skipping the swap there and returning lets {@code run} acknowledge the overwrite with a + * success for a table that holds nothing it wrote, which is the one outcome a client must not be told; + * {@link InsertOverwriteTableCommand} raises instead, so the caller's catch drops the temp partitions and + * the statement fails. This pins that branch: the table a lock could not be taken on has to be named in + * the failure. + * + * <p>Driven directly rather than through a statement because the window needs a DROP concurrent with the + * swap, which a regression test cannot place without an injection point of its own; the condition the + * branch reads -- {@code writeLockIfExist()} answering false -- is what the mock supplies. + */ +public class InsertOverwriteDroppedTargetTest { + + @Test + public void targetDroppedBeforeTheSwapFailsTheOverwrite() { + InsertOverwriteTableCommand command = new InsertOverwriteTableCommand( + Mockito.mock(LogicalPlan.class), Optional.empty(), Optional.empty(), Optional.empty()); + OlapTable droppedTarget = Mockito.mock(OlapTable.class); + Mockito.when(droppedTarget.getName()).thenReturn("iot_dropped_target"); + Mockito.when(droppedTarget.writeLockIfExist()).thenReturn(false); + ConnectContext ctx = Mockito.mock(ConnectContext.class); + Mockito.when(ctx.getQueryIdentifier()).thenReturn("stmt[1, query-id]"); + + UserException thrown = Assertions.assertThrows(UserException.class, + () -> Deencapsulation.invoke(command, "publishTheOverwrite", droppedTarget, Review Comment: My slip, and you are right. The previous commit's refactor changed `publishTheOverwrite`'s signature and I did not re-run the unit test afterwards. Reproduced exactly as described: `IllegalArgumentException: No compatible method found: publishTheOverwrite(OlapTable, ArrayList, ArrayList, OlapInsertCommandContext, ConnectContext)`, so `assertThrows(UserException.class)` failed before reaching the branch. Fixed the way you suggest: the test invokes the current method with a publication callback and asserts the callback was not reached (`assertFalse(published.get())`). The callback interface is package-private so a test in the same package can name it. `Tests run: 1, Failures: 0, Errors: 0` on this head. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/InsertOverwriteTableCommand.java: ########## @@ -257,45 +303,73 @@ public void run(ConnectContext ctx, StmtExecutor executor) throws Exception { } if (isAutoDetectOverwrite(getLogicalQuery())) { // taskId here is a group id. it contains all replace tasks made and registered in rpc process. - taskId = insertOverwriteManager.registerTaskGroup(targetTable); + final long groupId = insertOverwriteManager.registerTaskGroup(targetTable); + taskId = groupId; // When inserting, BE will call to replace partition by FrontendService. FE will register new temp // partitions and return. for transactional, the replacement will really occur when insert successed, // i.e. `insertInto` finished. then we call taskGroupSuccess to make replacement. - insertIntoAutoDetect(ctx, executor, taskId); - insertOverwriteManager.taskGroupSuccess(taskId, (OlapTable) targetTable, isForceDropPartition()); + InsertCommandContext insertCtx = insertIntoAutoDetect(ctx, executor, groupId); + if (isCancelled.get() && !insertCtx.hasCommitted()) { + // The load committed nothing -- an empty plan takes the path that begins no transaction -- + // so the cancellation still has everything to take back: the catch drops the group's temp + // partitions (an empty plan registers none), and the statement fails rather than reporting + // a replacement the client cancelled. A cancellation landing after this check, while the + // swap waits for the table lock, is taken up again by publishTheOverwrite below. + throw cancelledBeforeTheRowsWereCommitted("after a load that committed nothing", ctx); + } + publishTheOverwrite(targetTable, insertCtx, ctx, + () -> insertOverwriteManager.taskGroupSuccess(groupId, (OlapTable) targetTable, Review Comment: Confirmed and fixed the way you describe. I checked the durability this path actually uses before answering: `BDBEnvironment` builds `Durability(master_sync_policy, replica_sync_policy, replica_ack_policy)` from the defaults `SYNC / SYNC / SIMPLE_MAJORITY`, so every `logEdit` in that bookkeeping waits for a replica acknowledgement -- and my wrapper was holding the target's write lock across 2N of them. `taskGroupSuccess` is now split into `replacePartitionsOfTaskGroup` (the replacement) and `finishTaskGroup` (the per-task journaling and the task cleanup). The overwrite calls the replacement inside `publishTheOverwrite` -- under the lock, with the last-chance cancellation check -- and the bookkeeping after it returns. The owner-side entry (`FrontendServiceImpl` -> `InsertOverwriteManager.taskGroupSuccess`) keeps calling the composed method, and for a remote target the manager's replacement call is the existing RPC (the owner runs the whole sequence behind it) with `finishTaskGroup` a no-op there. So the lock again covers only the replacement and the last check, not the journal waits. -- 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]
