From 2a0ac05ce7e23bc0788ee5fcbf98b7f11ed62de4 Mon Sep 17 00:00:00 2001 From: pengxiangyu Date: Wed, 22 Jun 2022 15:28:04 +0800 Subject: [PATCH] [fix] Fix duplicate code for PropertyAnalyzer.analyzeDataProperty() (#10190) PropertyAnalyzer.analyzeDataProperty(Map properties, final DataProperty oldDataProperty) has something not suitable. Parameter oldDataProperty is the old DataProperty, properties should be used to replace some of its members. If properties has no some members, old ones need to be left, but not be set to default value. Function modifyPartitionsProperty() uses analyzeDataProperty(), but create a new DataProperty again, it is duplicate. --- .../java/org/apache/doris/alter/Alter.java | 19 ++----- .../doris/common/util/PropertyAnalyzer.java | 55 ++++++++----------- 2 files changed, 27 insertions(+), 47 deletions(-) diff --git a/fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java b/fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java index e4da3cb4e3..bc4159cd8a 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java +++ b/fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java @@ -24,7 +24,6 @@ import org.apache.doris.analysis.AlterTableStmt; import org.apache.doris.analysis.AlterViewStmt; import org.apache.doris.analysis.ColumnRenameClause; import org.apache.doris.analysis.CreateMaterializedViewStmt; -import org.apache.doris.analysis.DateLiteral; import org.apache.doris.analysis.DropMaterializedViewStmt; import org.apache.doris.analysis.DropPartitionClause; import org.apache.doris.analysis.ModifyColumnCommentClause; @@ -52,7 +51,6 @@ import org.apache.doris.catalog.PartitionInfo; import org.apache.doris.catalog.ReplicaAllocation; import org.apache.doris.catalog.Table; import org.apache.doris.catalog.TableIf.TableType; -import org.apache.doris.catalog.Type; import org.apache.doris.catalog.View; import org.apache.doris.common.AnalysisException; import org.apache.doris.common.DdlException; @@ -61,7 +59,6 @@ import org.apache.doris.common.UserException; import org.apache.doris.common.util.DynamicPartitionUtil; import org.apache.doris.common.util.MetaLockUtils; import org.apache.doris.common.util.PropertyAnalyzer; -import org.apache.doris.common.util.TimeUtils; import org.apache.doris.persist.AlterViewInfo; import org.apache.doris.persist.BatchModifyPartitionsInfo; import org.apache.doris.persist.ModifyCommentOperationLog; @@ -81,7 +78,6 @@ import org.apache.logging.log4j.Logger; import java.util.Arrays; import java.util.Comparator; -import java.util.HashMap; import java.util.List; import java.util.Map; @@ -684,17 +680,10 @@ public class Alter { // 4. data property // 4.1 get old data property from partition DataProperty dataProperty = partitionInfo.getDataProperty(partition.getId()); - // 4.2 combine the old properties with new ones - Map newProperties = new HashMap<>(); - newProperties.put(PropertyAnalyzer.PROPERTIES_STORAGE_MEDIUM, dataProperty.getStorageMedium().name()); - DateLiteral dateLiteral = new DateLiteral(dataProperty.getCooldownTimeMs(), - TimeUtils.getTimeZone(), Type.DATETIME); - newProperties.put(PropertyAnalyzer.PROPERTIES_STORAGE_COOLDOWN_TIME, dateLiteral.getStringValue()); - newProperties.put(PropertyAnalyzer.PROPERTIES_REMOTE_STORAGE_POLICY, dataProperty.getRemoteStoragePolicy()); - newProperties.putAll(properties); - // 4.3 analyze new properties - DataProperty newDataProperty = - PropertyAnalyzer.analyzeDataProperty(newProperties, DataProperty.DEFAULT_DATA_PROPERTY); + Map modifiedProperties = Maps.newHashMap(); + modifiedProperties.putAll(properties); + // 4.2 analyze new properties + DataProperty newDataProperty = PropertyAnalyzer.analyzeDataProperty(modifiedProperties, dataProperty); // 1. date property if (newDataProperty != null) { diff --git a/fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java b/fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java index 84b29b9803..144576af6d 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java +++ b/fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java @@ -44,6 +44,7 @@ import org.apache.doris.thrift.TTabletType; import com.google.common.base.Preconditions; import com.google.common.base.Strings; import com.google.common.collect.Sets; +import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -111,24 +112,28 @@ public class PropertyAnalyzer { private static final double MAX_FPP = 0.05; private static final double MIN_FPP = 0.0001; - public static DataProperty analyzeDataProperty(Map properties, DataProperty oldDataProperty) + /** + * check and replace members of DataProperty by properties. + * + * @param properties key->value for members to change. + * @param oldDataProperty old DataProperty + * @return new DataProperty + * @throws AnalysisException property has invalid key->value + */ + public static DataProperty analyzeDataProperty(Map properties, final DataProperty oldDataProperty) throws AnalysisException { if (properties == null || properties.isEmpty()) { return oldDataProperty; } - TStorageMedium storageMedium = null; - long cooldownTimeStamp = DataProperty.MAX_COOLDOWN_TIME_MS; - String remoteStoragePolicy = ""; + TStorageMedium storageMedium = oldDataProperty.getStorageMedium(); + long cooldownTimeStamp = oldDataProperty.getCooldownTimeMs(); + String remoteStoragePolicy = oldDataProperty.getRemoteStoragePolicy(); - boolean hasMedium = false; - boolean hasCooldown = false; - boolean hasRemoteStoragePolicy = false; for (Map.Entry entry : properties.entrySet()) { String key = entry.getKey(); String value = entry.getValue(); - if (!hasMedium && key.equalsIgnoreCase(PROPERTIES_STORAGE_MEDIUM)) { - hasMedium = true; + if (key.equalsIgnoreCase(PROPERTIES_STORAGE_MEDIUM)) { if (value.equalsIgnoreCase(TStorageMedium.SSD.name())) { storageMedium = TStorageMedium.SSD; } else if (value.equalsIgnoreCase(TStorageMedium.HDD.name())) { @@ -136,40 +141,26 @@ public class PropertyAnalyzer { } else { throw new AnalysisException("Invalid storage medium: " + value); } - } else if (!hasCooldown && key.equalsIgnoreCase(PROPERTIES_STORAGE_COOLDOWN_TIME)) { + } else if (key.equalsIgnoreCase(PROPERTIES_STORAGE_COOLDOWN_TIME)) { DateLiteral dateLiteral = new DateLiteral(value, Type.DATETIME); cooldownTimeStamp = dateLiteral.unixTimestamp(TimeUtils.getTimeZone()); - if (cooldownTimeStamp != DataProperty.MAX_COOLDOWN_TIME_MS) { - hasCooldown = true; - } - } else if (!hasRemoteStoragePolicy && key.equalsIgnoreCase(PROPERTIES_REMOTE_STORAGE_POLICY)) { - if (!Strings.isNullOrEmpty(value)) { - hasRemoteStoragePolicy = true; - remoteStoragePolicy = value; - } + } else if (key.equalsIgnoreCase(PROPERTIES_REMOTE_STORAGE_POLICY)) { + remoteStoragePolicy = value; } } // end for properties - // Check properties - - if (!hasCooldown && !hasMedium && !hasRemoteStoragePolicy) { - return oldDataProperty; - } - properties.remove(PROPERTIES_STORAGE_MEDIUM); properties.remove(PROPERTIES_STORAGE_COOLDOWN_TIME); properties.remove(PROPERTIES_REMOTE_STORAGE_POLICY); - if (hasCooldown && !hasMedium) { - throw new AnalysisException("Invalid data property. storage medium property is not found"); - } - - if (storageMedium == TStorageMedium.HDD && hasCooldown) { + if (storageMedium == TStorageMedium.HDD) { cooldownTimeStamp = DataProperty.MAX_COOLDOWN_TIME_MS; LOG.info("Can not assign cool down timestamp to HDD storage medium, ignore user setting."); - hasCooldown = false; } + boolean hasCooldown = cooldownTimeStamp != DataProperty.MAX_COOLDOWN_TIME_MS; + boolean hasRemoteStoragePolicy = StringUtils.isNotEmpty(remoteStoragePolicy); + long currentTimeMs = System.currentTimeMillis(); if (storageMedium == TStorageMedium.SSD && hasCooldown) { if (cooldownTimeStamp <= currentTimeMs) { @@ -183,7 +174,7 @@ public class PropertyAnalyzer { } if (hasRemoteStoragePolicy) { - // check remote resource + // check remote storage policy StoragePolicy checkedPolicy = new StoragePolicy(PolicyTypeEnum.STORAGE, remoteStoragePolicy); Policy policy = Catalog.getCurrentCatalog().getPolicyMgr().getPolicy(checkedPolicy); if (!(policy instanceof StoragePolicy)) { @@ -195,7 +186,7 @@ public class PropertyAnalyzer { if (storagePolicy.getCooldownDatetime().getTime() <= currentTimeMs) { throw new AnalysisException("Remote storage cool down time should later than now"); } - if (hasCooldown && (storagePolicy.getCooldownDatetime().getTime() <= cooldownTimeStamp)) { + if (hasCooldown && storagePolicy.getCooldownDatetime().getTime() <= cooldownTimeStamp) { throw new AnalysisException("`remote_storage_cooldown_time`" + " should later than `storage_cooldown_time`."); }