From f5f9ec1bca3b336ea6ac2beb0e5e8d37a4f2bc0f Mon Sep 17 00:00:00 2001 From: deardeng Date: Thu, 23 Jul 2026 15:39:52 +0800 Subject: [PATCH] [fix](replica) Resolve conflicting default replica properties (#65836) Problem Summary: ALTER TABLE updates to default.replication_allocation could leave the legacy default.replication_num property in table metadata. Because replica analysis checks replication_num first, SHOW CREATE TABLE and later consumers could observe the wrong default allocation. Remove the mutually exclusive property when applying an update, prefer replication_allocation when cleaning historical metadata, and cover allocation updates, metadata cleanup, and reverse numeric updates. (cherry picked from commit 8a1bf788e290dc5832c4bd432a34d0e8b4c42906) --- .../catalog/InternalSchemaInitializer.java | 8 +- .../apache/doris/catalog/TableProperty.java | 28 +++-- .../doris/alter/InternalSchemaAlterTest.java | 5 + .../doris/catalog/TablePropertyTest.java | 113 ++++++++++++++++++ 4 files changed, 144 insertions(+), 10 deletions(-) create mode 100644 fe/fe-core/src/test/java/org/apache/doris/catalog/TablePropertyTest.java diff --git a/fe/fe-core/src/main/java/org/apache/doris/catalog/InternalSchemaInitializer.java b/fe/fe-core/src/main/java/org/apache/doris/catalog/InternalSchemaInitializer.java index d9c7db417581fd..0dc27a15581957 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/catalog/InternalSchemaInitializer.java +++ b/fe/fe-core/src/main/java/org/apache/doris/catalog/InternalSchemaInitializer.java @@ -256,10 +256,12 @@ public static void modifyTblReplicaCount(Database database, String tblName) { InternalCatalog.INTERNAL_CATALOG_NAME, StatisticConstants.DB_NAME, tbl.getName()); - // 1. modify table's default replica num + // 1. modify table's default replica allocation Map props = new HashMap<>(); - props.put("default." + PropertyAnalyzer.PROPERTIES_REPLICATION_NUM, - "" + StatisticConstants.STATISTIC_INTERNAL_TABLE_REPLICA_NUM); + ReplicaAllocation replicaAllocation = new ReplicaAllocation( + (short) StatisticConstants.STATISTIC_INTERNAL_TABLE_REPLICA_NUM); + props.put("default." + PropertyAnalyzer.PROPERTIES_REPLICATION_ALLOCATION, + replicaAllocation.toCreateStmt()); Env.getCurrentEnv().modifyTableDefaultReplicaAllocation(database, tbl, props); // 2. modify each partition's replica num diff --git a/fe/fe-core/src/main/java/org/apache/doris/catalog/TableProperty.java b/fe/fe-core/src/main/java/org/apache/doris/catalog/TableProperty.java index ab770808d52792..84eac8baf74511 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/catalog/TableProperty.java +++ b/fe/fe-core/src/main/java/org/apache/doris/catalog/TableProperty.java @@ -53,6 +53,10 @@ */ public class TableProperty implements GsonPostProcessable { private static final Logger LOG = LogManager.getLogger(TableProperty.class); + private static final String DEFAULT_REPLICATION_NUM = + "default." + PropertyAnalyzer.PROPERTIES_REPLICATION_NUM; + private static final String DEFAULT_REPLICATION_ALLOCATION = + "default." + PropertyAnalyzer.PROPERTIES_REPLICATION_ALLOCATION; @SerializedName(value = "properties") private Map properties; @@ -612,10 +616,23 @@ public TableProperty buildInvertedIndexFileStorageFormat() { } public void modifyTableProperties(Map modifyProperties) { + // Compatibility note: ModifyTablePropertyOperationLog persists only properties to set, not keys removed + // here. Keep its payload unchanged for this legacy repair. During a rolling FE upgrade, alter these + // properties on a table that already contains both legacy keys only after all FEs have been upgraded; + // otherwise old and new FEs may apply different effective replica settings. + removeConflictingDefaultReplicaProperty(modifyProperties); properties.putAll(modifyProperties); removeDuplicateReplicaNumProperty(); } + private void removeConflictingDefaultReplicaProperty(Map modifyProperties) { + if (modifyProperties.containsKey(DEFAULT_REPLICATION_ALLOCATION)) { + properties.remove(DEFAULT_REPLICATION_NUM); + } else if (modifyProperties.containsKey(DEFAULT_REPLICATION_NUM)) { + properties.remove(DEFAULT_REPLICATION_ALLOCATION); + } + } + public void modifyDataSortInfoProperties(DataSortInfo dataSortInfo) { properties.put(DataSortInfo.DATA_SORT_TYPE, String.valueOf(dataSortInfo.getSortType())); properties.put(DataSortInfo.DATA_SORT_COL_NUM, String.valueOf(dataSortInfo.getColNum())); @@ -624,8 +641,8 @@ public void modifyDataSortInfoProperties(DataSortInfo dataSortInfo) { public void setReplicaAlloc(ReplicaAllocation replicaAlloc) { this.replicaAlloc = replicaAlloc; // set it to "properties" so that this info can be persisted - properties.put("default." + PropertyAnalyzer.PROPERTIES_REPLICATION_ALLOCATION, - replicaAlloc.toCreateStmt()); + properties.remove(DEFAULT_REPLICATION_NUM); + properties.put(DEFAULT_REPLICATION_ALLOCATION, replicaAlloc.toCreateStmt()); } public ReplicaAllocation getReplicaAllocation() { @@ -810,11 +827,8 @@ public void gsonPostProcess() throws IOException { buildTDEAlgorithm(); } - // For some historical reason, - // both "dynamic_partition.replication_num" and "dynamic_partition.replication_allocation" - // may be exist in "properties". we need remove the "dynamic_partition.replication_num", or it will always replace - // the "dynamic_partition.replication_allocation", - // result in unable to set "dynamic_partition.replication_allocation". + // Historical dynamic partition metadata may contain both replica properties. Keep the allocation form by + // removing replication_num because the analyzer checks it first. private void removeDuplicateReplicaNumProperty() { if (properties.containsKey(DynamicPartitionProperty.REPLICATION_NUM) && properties.containsKey(DynamicPartitionProperty.REPLICATION_ALLOCATION)) { diff --git a/fe/fe-core/src/test/java/org/apache/doris/alter/InternalSchemaAlterTest.java b/fe/fe-core/src/test/java/org/apache/doris/alter/InternalSchemaAlterTest.java index 122014f0e8b2c2..3ad88c49c408bd 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/alter/InternalSchemaAlterTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/alter/InternalSchemaAlterTest.java @@ -28,6 +28,7 @@ import org.apache.doris.common.AnalysisException; import org.apache.doris.common.Config; import org.apache.doris.common.FeConstants; +import org.apache.doris.common.util.PropertyAnalyzer; import org.apache.doris.plugin.audit.AuditLoader; import org.apache.doris.statistics.StatisticConstants; import org.apache.doris.utframe.TestWithFeService; @@ -67,6 +68,10 @@ private void checkReplicationNum(Database db, String tblName) throws AnalysisExc PartitionInfo partitionInfo = olapTable.getPartitionInfo(); Assertions.assertEquals((short) 3, olapTable.getTableProperty().getReplicaAllocation().getTotalReplicaNum(), tblName); + Assertions.assertFalse(olapTable.getTableProperty().getProperties() + .containsKey("default." + PropertyAnalyzer.PROPERTIES_REPLICATION_NUM), tblName); + Assertions.assertTrue(olapTable.getTableProperty().getProperties() + .containsKey("default." + PropertyAnalyzer.PROPERTIES_REPLICATION_ALLOCATION), tblName); for (Partition partition : olapTable.getPartitions()) { Assertions.assertEquals((short) 3, partitionInfo.getReplicaAllocation(partition.getId()).getTotalReplicaNum()); diff --git a/fe/fe-core/src/test/java/org/apache/doris/catalog/TablePropertyTest.java b/fe/fe-core/src/test/java/org/apache/doris/catalog/TablePropertyTest.java new file mode 100644 index 00000000000000..ca13c727691e96 --- /dev/null +++ b/fe/fe-core/src/test/java/org/apache/doris/catalog/TablePropertyTest.java @@ -0,0 +1,113 @@ +// 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.catalog; + +import org.apache.doris.common.util.PropertyAnalyzer; +import org.apache.doris.resource.Tag; + +import com.google.common.collect.Maps; +import org.junit.Assert; +import org.junit.Test; + +import java.io.IOException; +import java.util.Map; + +public class TablePropertyTest { + private static final String DEFAULT_REPLICATION_NUM = + "default." + PropertyAnalyzer.PROPERTIES_REPLICATION_NUM; + private static final String DEFAULT_REPLICATION_ALLOCATION = + "default." + PropertyAnalyzer.PROPERTIES_REPLICATION_ALLOCATION; + private static final String REPLICATION_ALLOCATION = + "tag.location.group_0: 1, tag.location.group_1: 1, tag.location.group_2: 1"; + + @Test + public void testModifyDefaultReplicaAllocationRemovesLegacyReplicationNum() { + Map properties = Maps.newHashMap(); + properties.put(DEFAULT_REPLICATION_NUM, "3"); + TableProperty tableProperty = new TableProperty(properties); + tableProperty.buildReplicaAllocation(); + + Map modifiedProperties = Maps.newHashMap(); + modifiedProperties.put(DEFAULT_REPLICATION_ALLOCATION, REPLICATION_ALLOCATION); + tableProperty.modifyTableProperties(modifiedProperties); + tableProperty.buildReplicaAllocation(); + + assertReplicaAllocationWins(tableProperty); + } + + @Test + public void testDeserializeConflictingDefaultReplicaPropertiesPreservesNumericPrecedence() throws IOException { + Map properties = Maps.newHashMap(); + properties.put(DEFAULT_REPLICATION_NUM, "3"); + properties.put(DEFAULT_REPLICATION_ALLOCATION, REPLICATION_ALLOCATION); + TableProperty tableProperty = new TableProperty(properties); + + tableProperty.gsonPostProcess(); + + Assert.assertTrue(tableProperty.getProperties().containsKey(DEFAULT_REPLICATION_NUM)); + Assert.assertTrue(tableProperty.getProperties().containsKey(DEFAULT_REPLICATION_ALLOCATION)); + Assert.assertEquals(Short.valueOf((short) 3), + tableProperty.getReplicaAllocation().getReplicaNumByTag(Tag.DEFAULT_BACKEND_TAG)); + } + + @Test + public void testResetPropertiesForRestoreRemovesLegacyReplicationNum() { + Map properties = Maps.newHashMap(); + properties.put(DEFAULT_REPLICATION_NUM, "3"); + TableProperty tableProperty = new TableProperty(properties); + tableProperty.buildReplicaAllocation(); + + ReplicaAllocation restoredReplicaAllocation = new ReplicaAllocation((short) 2); + tableProperty.resetPropertiesForRestore(false, false, restoredReplicaAllocation); + + Assert.assertFalse(tableProperty.getProperties().containsKey(DEFAULT_REPLICATION_NUM)); + Assert.assertEquals(restoredReplicaAllocation.toCreateStmt(), + tableProperty.getProperties().get(DEFAULT_REPLICATION_ALLOCATION)); + Assert.assertEquals((short) 2, tableProperty.getReplicaAllocation().getTotalReplicaNum()); + } + + @Test + public void testModifyDefaultReplicationNumRemovesExistingAllocation() { + Map properties = Maps.newHashMap(); + properties.put(DEFAULT_REPLICATION_ALLOCATION, REPLICATION_ALLOCATION); + TableProperty tableProperty = new TableProperty(properties); + tableProperty.buildReplicaAllocation(); + + Map modifiedProperties = Maps.newHashMap(); + modifiedProperties.put(DEFAULT_REPLICATION_NUM, "2"); + tableProperty.modifyTableProperties(modifiedProperties); + tableProperty.buildReplicaAllocation(); + + Assert.assertFalse(tableProperty.getProperties().containsKey(DEFAULT_REPLICATION_ALLOCATION)); + Assert.assertEquals(Short.valueOf((short) 2), + tableProperty.getReplicaAllocation().getReplicaNumByTag(Tag.DEFAULT_BACKEND_TAG)); + } + + private void assertReplicaAllocationWins(TableProperty tableProperty) { + Assert.assertFalse(tableProperty.getProperties().containsKey(DEFAULT_REPLICATION_NUM)); + Assert.assertEquals(Short.valueOf((short) 1), + tableProperty.getReplicaAllocation() + .getReplicaNumByTag(Tag.createNotCheck(Tag.TYPE_LOCATION, "group_0"))); + Assert.assertEquals(Short.valueOf((short) 1), + tableProperty.getReplicaAllocation() + .getReplicaNumByTag(Tag.createNotCheck(Tag.TYPE_LOCATION, "group_1"))); + Assert.assertEquals(Short.valueOf((short) 1), + tableProperty.getReplicaAllocation() + .getReplicaNumByTag(Tag.createNotCheck(Tag.TYPE_LOCATION, "group_2"))); + } +}