github-actions[bot] commented on code in PR #65836:
URL: https://github.com/apache/doris/pull/65836#discussion_r3627480468
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/TableProperty.java:
##########
@@ -678,10 +678,22 @@ public TableProperty
buildInvertedIndexFileStorageFormat() {
}
public void modifyTableProperties(Map<String, String> modifyProperties) {
+ removeConflictingReplicaProperty(modifyProperties,
Review Comment:
[P1] Enforce the invariant in the restore setter too
This only makes the map overload conflict-aware, but
`resetPropertiesForRestore(..., reserveReplica=false, ...)` calls
`setReplicaAlloc()` on an existing `TableProperty`. That setter writes
`default.replication_allocation` directly without removing a legacy
`default.replication_num`, so restoring numeric-only old metadata still
persists both keys: the leader uses the target allocation field, while an older
rolling-upgrade follower can rebuild the numeric value. Please route
`setReplicaAlloc()` through the same mutual-exclusion primitive (or remove the
numeric key there) and add a restore-path test.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/TableProperty.java:
##########
@@ -968,16 +980,19 @@ public void gsonPostProcess() throws IOException {
buildColumnSeqMapping();
}
- // 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".
+ // Both replication_num and replication_allocation may exist in properties
loaded from old metadata.
+ // Prefer the canonical replication_allocation form because
replication_num is checked first by the analyzer.
private void removeDuplicateReplicaNumProperty() {
if (properties.containsKey(DynamicPartitionProperty.REPLICATION_NUM)
&&
properties.containsKey(DynamicPartitionProperty.REPLICATION_ALLOCATION)) {
properties.remove(DynamicPartitionProperty.REPLICATION_NUM);
}
+ String defaultReplicationNum = "default." +
PropertyAnalyzer.PROPERTIES_REPLICATION_NUM;
+ String defaultReplicationAllocation = "default." +
PropertyAnalyzer.PROPERTIES_REPLICATION_ALLOCATION;
+ if (properties.containsKey(defaultReplicationNum)
+ && properties.containsKey(defaultReplicationAllocation)) {
+ properties.remove(defaultReplicationNum);
Review Comment:
[P1] Preserve newer numeric values in historical images
Conflicting snapshots do not guarantee that allocation is newer.
`InternalSchemaInitializer.modifyTblReplicaCount` writes raw
`default.replication_num=3` for partitioned internal tables after creation has
stored `default.replication_allocation` (commonly 1). Before this patch both
keys remain and the analyzer deliberately selects numeric first. On the first
image load with this branch, the newer 3 is deleted and
`buildReplicaAllocation()` rebuilds the older allocation, so the
reported/default allocation and future partition inheritance can revert. Please
preserve numeric-first semantics for ambiguous historical images (or migrate
only when chronology is provable), canonicalize future producers, and add a
numeric-newer upgrade test.
--
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]
