Re: [PR] [fix](cloud)Fix `create dynamic table` race with `insert overwrite` [doris]

["dataroaring (via GitHub)" <gi...@apache.org>]

dataroaring commented on code in PR #59489:
URL: https://github.com/apache/doris/pull/59489#discussion_r2658181490


##########
fe/fe-core/src/main/java/org/apache/doris/clone/DynamicPartitionScheduler.java:
##########
@@ -847,28 +843,56 @@ private void cloudBatchAfterCreatePartitions(boolean executeFirstTime, List<Part
                     succeedPartitionIds, indexIds, true /* isCreateTable */, false /* isBatchCommit */);
             LOG.info("begin write edit log to add partitions in batch, "
                     + "numPartitions: {}, db: {}, table: {}, tableId: {}",
-                    partsInfo.size(), db.getFullName(), tableName, olapTable.getId());
+                    batchPartsInfo.size(), db.getFullName(), tableName, olapTable.getId());
             // ATTN: here, edit log must after commit cloud partition,
             // prevent commit RPC failure from causing data loss
             if (DebugPointUtil.isEnable("FE.DynamicPartitionScheduler.before.logEditPartitions")) {
                 LOG.info("debug point FE.DynamicPartitionScheduler.before.logEditPartitions, throw e");
                 // committed, but not log edit
                 throw new Exception("debug point FE.DynamicPartitionScheduler.before.commitCloudPartition");
             }
-            for (int i = 0; i < partsInfo.size(); i++) {
-                Env.getCurrentEnv().getEditLog().logAddPartition(partsInfo.get(i));
-                if (DebugPointUtil.isEnable("FE.DynamicPartitionScheduler.in.logEditPartitions")) {
-                    if (i == partsInfo.size() / 2) {
-                        LOG.info("debug point FE.DynamicPartitionScheduler.in.logEditPartitions, throw e");
-                        // committed, but log some edit, others failed
-                        throw new Exception("debug point FE.DynamicPartitionScheduler"
-                            + ".in.commitCloudPartition");
+
+            for (int i = 0; i < batchPartsInfo.size(); i++) {
+                // get table write lock to add partition, edit log and modify table state must be atomic
+                olapTable.writeLockOrDdlException();
+                try {
+                    boolean isTempPartition = addPartitionOps.get(i).isTempPartition();
+                    Partition toAddPartition = batchPartsInfo.get(i).second;
+                    String partitionName = toAddPartition.getName();
+                    // ATTN: Check here to see if the newly created dynamic
+                    // partition has already been added by another process.
+                    // If it has, do not add this dynamic partition again,
+                    // and call `onErasePartition` to clean up any remaining information.
+                    Partition checkIsAdded = olapTable.getPartition(partitionName, isTempPartition);
+                    if (checkIsAdded != null) {
+                        LOG.warn("dynamic partition has been added, skip it. "
+                                + "db: {}, table: {}, partition: {}, tableId: {}",
+                                db.getFullName(), tableName, partitionName, olapTable.getId());
+                        Env.getCurrentEnv().onErasePartition(toAddPartition);

Review Comment:
   There is remote rpc inside, we'd better do not hold lock doing rpc.



-- 
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: commits-unsubscribe@doris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org