github-actions[bot] commented on code in PR #66603:
URL: https://github.com/apache/doris/pull/66603#discussion_r3748096285
##########
fe/fe-core/src/test/java/org/apache/doris/catalog/TablePropertyTest.java:
##########
@@ -116,6 +120,26 @@ public void testCompleteDynamicPartitionIsBuilt() {
Assert.assertEquals(1,
tableProperty.getDynamicPartitionProperty().getBuckets());
}
+ @Test
+ public void testStorageMediumIsCaseInsensitiveAfterSerialization() {
+ List<String> storageMediumValues = Arrays.asList("hdd", "HDD", "HdD",
"ssd", "SSD", "SsD");
+ for (String storageMediumValue : storageMediumValues) {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(PropertyAnalyzer.PROPERTIES_STORAGE_MEDIUM,
storageMediumValue);
+ TableProperty tableProperty = new
TableProperty(properties).buildStorageMedium();
+
+ String serialized = GsonUtils.GSON.toJson(tableProperty);
+ TableProperty deserialized = GsonUtils.GSON.fromJson(serialized,
TableProperty.class);
+
+ TStorageMedium expectedStorageMedium =
storageMediumValue.equalsIgnoreCase("hdd")
+ ? TStorageMedium.HDD : TStorageMedium.SSD;
+ Assert.assertEquals(expectedStorageMedium,
tableProperty.getStorageMedium());
+ Assert.assertEquals(expectedStorageMedium,
deserialized.getStorageMedium());
+ Assert.assertEquals(storageMediumValue,
Review Comment:
[P1] Canonicalize the value before persisting it
This assertion preserves the raw lowercase/mixed-case spelling in metadata,
but `buildStorageMedium()` is also used by live `Env.modifyTableProperties`,
whose input map is then written unchanged to `ModifyTablePropertyOperationLog`.
During a rolling FE upgrade, an upgraded follower can become master while old
peers remain; after `ALTER ... storage_medium='hdd'` succeeds on that master,
an old peer replays the log through the base case-sensitive `valueOf("hdd")`,
and `EditLog` treats that exception as fatal. A checkpoint containing the same
raw property has the same problem. Please canonicalize valid values in both the
stored TableProperty and the journal payload (while accepting legacy spellings
on read), and assert the canonical persisted form here.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/TableProperty.java:
##########
@@ -545,7 +546,7 @@ public TableProperty buildStorageMedium() {
if (Strings.isNullOrEmpty(storageMediumStr)) {
storageMedium = null;
} else {
- storageMedium = TStorageMedium.valueOf(storageMediumStr);
+ storageMedium =
TStorageMedium.valueOf(storageMediumStr.toUpperCase(Locale.ROOT));
Review Comment:
[P2] Keep case folding within the supported table media
`TStorageMedium` also contains `S3` and `REMOTE_CACHE`, so this enum-wide
normalization makes `ALTER TABLE ... SET ("storage_medium"="s3")` (and
`remote_cache`) succeed and persist. The ALTER validation path does not check
the value, but automatic partition creation later copies this table default
through `PartitionExprUtil` into `PropertyAnalyzer.analyzeDataProperty`, which
accepts only HDD/SSD and fails with `Invalid storage medium: S3`. Please
validate live table-property input against HDD/SSD (while still repairing
legacy metadata here), and add negative cases for the other enum members.
--
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]
