github-actions[bot] commented on code in PR #65053:
URL: https://github.com/apache/doris/pull/65053#discussion_r3507831161
##########
fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java:
##########
@@ -916,6 +920,56 @@ private void processRename(Database db, Table table,
List<AlterOp> alterOps) thr
}
}
+ private void checkModifyPartitionStoragePolicyResource(OlapTable
olapTable, List<String> partitionNames,
+ Map<String, String> properties, boolean isTempPartition) throws
AnalysisException {
+ if (!PropertyAnalyzer.hasStoragePolicy(properties)) {
+ return;
+ }
+
+ String newStoragePolicy =
PropertyAnalyzer.analyzeStoragePolicy(properties);
+ if (Strings.isNullOrEmpty(newStoragePolicy)) {
+ return;
+ }
Review Comment:
This needs to run the normal target-policy validation before the new
resource precheck or before any BE update task is sent.
`default_storage_policy` is created at startup without a storage resource, and
the existing rejection for that state is `checkStoragePolicyExist()` /
`checkDefaultStoragePolicyValid()`. This helper only does `findPolicy()`: if
the partition already has a real old policy, `newResource` is null and
`newResource.equals(oldResource)` throws an internal NPE; if the partition has
no old policy, this loop skips validation, `updatePartitionsProperties()` can
push the default policy id to BE tablets, and FE only rejects later in
`modifyPartitionsProperty()`. Please reuse
`checkStoragePolicyExist(newStoragePolicy)` before this helper returns or
compares resources, and add a regression case for `MODIFY PARTITION` with
`default_storage_policy` while the default policy has not been initialized.
##########
regression-test/suites/cold_heat_separation/empty_table_use_policy/modify_partition_add_policy.groovy:
##########
@@ -112,6 +112,61 @@ suite("add_table_policy_by_modify_partition") {
"cooldown_datetime" = "$cooldownTime"
);
"""
+ sql """ALTER STORAGE POLICY tmp2 PROPERTIES("cooldown_datetime" =
"$cooldownTime")"""
+
+ try_sql """
+ CREATE RESOURCE IF NOT EXISTS
"test_modify_partition_table_use_resource_diff"
+ PROPERTIES(
+ "type"="s3",
+ "AWS_REGION" = "bj",
+ "AWS_ENDPOINT" = "bj.s3.comaaaa",
+ "AWS_ROOT_PATH" = "path/to/rootaaaa",
+ "AWS_SECRET_KEY" = "aaaa",
+ "AWS_ACCESS_KEY" = "bbba",
+ "AWS_BUCKET" = "test-bucket-diff",
+ "s3_validity_check" = "false"
+ );
+ """
+ try_sql """
+ CREATE STORAGE POLICY IF NOT EXISTS
created_create_table_partition_alter_policy_diff
+ PROPERTIES(
+ "storage_resource" = "test_modify_partition_table_use_resource_diff",
+ "cooldown_datetime" = "$cooldownTime"
+ );
+ """
+ sql """ALTER STORAGE POLICY
created_create_table_partition_alter_policy_diff PROPERTIES("cooldown_datetime"
= "$cooldownTime")"""
+
assertEquals(storage_exist.call("created_create_table_partition_alter_policy_diff"),
true)
+
+ def partitionsAfterSetPolicy = sql_return_maparray """
+ show partitions from create_table_partition
+ """
+ for (def par in partitionsAfterSetPolicy) {
+ assertTrue(par.RemoteStoragePolicy ==
"created_create_table_partition_alter_policy")
+ }
+
+ def alter_table_partition_diff_resource_result = try_sql """
+ ALTER TABLE create_table_partition MODIFY PARTITION (*)
SET("storage_policy"="created_create_table_partition_alter_policy_diff");
+ """
+ assertEquals(alter_table_partition_diff_resource_result, null)
+
+ def partitionsAfterFailedAlter = sql_return_maparray """
Review Comment:
This assertion does not exercise the behavior the PR is trying to protect.
Before this change, the failed cross-resource alter could already send
`UpdateTabletMetaInfoTask` to BE and then fail later in FE; `SHOW PARTITIONS`
would still display the old `RemoteStoragePolicy` because that column comes
from FE `DataProperty`, which is not updated when `analyzeDataProperty()`
throws. Please add coverage that would fail when the BE update is attempted
before validation, such as a focused FE/unit test around the
`updatePartitionsProperties()` call ordering or another observable BE-side
check, rather than only checking FE metadata after the failure.
--
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]
