luwei16 commented on code in PR #68094:
URL: https://github.com/apache/doris/pull/68094#discussion_r4034344310
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/alter/CloudSchemaChangeHandler.java:
##########
@@ -426,6 +427,18 @@ public void updateTableProperties(Database db, String
tableName, Map<String, Str
DynamicPartitionUtil.registerOrRemoveDynamicPartitionTable(db.getId(),
olapTable, false);
}
+ @Override
+ public void updatePartitionProperties(Database db, String tableName,
String partitionName,
+ long storagePolicyId, int isInMemory, BinlogConfig binlogConfig,
String compactionPolicy,
+ Map<String, Long> timeSeriesCompactionConfig, int
skipWriteIndexOnLoad,
+ int disableAutoCompaction, int
verticalCompactionNumColumnsPerGroup) throws UserException {
+ Preconditions.checkNotNull(binlogConfig);
+ UpdatePartitionMetaParam param = new UpdatePartitionMetaParam();
+ param.binlogConfig = binlogConfig;
+ param.type = UpdatePartitionMetaParam.TabletMetaType.BINLOG_CONFIG;
+ updateCloudPartitionMeta(db, tableName, partitionName, param);
Review Comment:
Correction: withdrawing this as a blocker for this PR. The formal-only
`olapTable.getPartitions()` snapshot is in the unchanged
`SchemaChangeHandler.updateBinlogConfig()` caller and predates this patch.
Under the requested regression-only attribution policy, this is a pre-existing
coverage gap to track separately, not a defect introduced by this diff.
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/alter/CloudSchemaChangeHandler.java:
##########
@@ -426,6 +427,18 @@ public void updateTableProperties(Database db, String
tableName, Map<String, Str
DynamicPartitionUtil.registerOrRemoveDynamicPartitionTable(db.getId(),
olapTable, false);
}
+ @Override
+ public void updatePartitionProperties(Database db, String tableName,
String partitionName,
Review Comment:
Correction: withdrawing this as a blocker for this PR. The ADD PARTITION
snapshot/publication protocol and its lack of a BinlogConfig fence are
unchanged by this patch. Under the requested regression-only attribution
policy, this is a pre-existing concurrency gap to track separately, not a
defect introduced by this diff.
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/alter/CloudSchemaChangeHandler.java:
##########
@@ -426,6 +427,18 @@ public void updateTableProperties(Database db, String
tableName, Map<String, Str
DynamicPartitionUtil.registerOrRemoveDynamicPartitionTable(db.getId(),
olapTable, false);
}
+ @Override
+ public void updatePartitionProperties(Database db, String tableName,
String partitionName,
+ long storagePolicyId, int isInMemory, BinlogConfig binlogConfig,
String compactionPolicy,
+ Map<String, Long> timeSeriesCompactionConfig, int
skipWriteIndexOnLoad,
+ int disableAutoCompaction, int
verticalCompactionNumColumnsPerGroup) throws UserException {
+ Preconditions.checkNotNull(binlogConfig);
+ UpdatePartitionMetaParam param = new UpdatePartitionMetaParam();
+ param.binlogConfig = binlogConfig;
Review Comment:
Correction: withdrawing this as a blocker for this PR. The snapshot,
lock-free remote phase, and final FE journal sequence are implemented by the
unchanged `SchemaChangeHandler.updateBinlogConfig()` path and predate this
patch. The new cloud persistence makes the existing risk visible, but it is not
a newly introduced concurrency model; track it separately rather than blocking
this PR under the requested attribution policy.
--
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]
