github-actions[bot] commented on code in PR #68094:
URL: https://github.com/apache/doris/pull/68094#discussion_r4033613562
##########
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:
[P2] Include temporary partitions in the binlog-config fanout. The caller
snapshots `olapTable.getPartitions()`, which explicitly excludes temp
partitions, even though ROW-binlog tables allow temp partitions and `REPLACE
PARTITION` later promotes the same tablet objects. An existing temp partition
therefore keeps the old TTL/max settings in MetaService; after replacement FE
reports the new table config while BE refresh continues to pull the old tablet
config. Please update all formal and temporary partitions (and test
alter-then-replace).
##########
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:
[P2] Fence partition publication against this config update. `ADD PARTITION`
copies the old `BinlogConfig`, creates its MetaService tablets outside the
table lock, and its final `metaChanged` check does not compare binlog config.
If that add overlaps this fanout, it is absent from the snapped partition list
and can publish old-config tablets after FE journals the new value, with no
later reconciliation. Add the config to publication validation or use a
versioned reconciliation/serialization step.
##########
be/src/cloud/cloud_tablet.cpp:
##########
@@ -1621,6 +1622,7 @@ Status CloudTablet::sync_meta() {
_tablet_meta->set_vertical_compaction_num_columns_per_group(
new_vertical_compaction_num_columns_per_group);
}
+ _tablet_meta->set_binlog_config(new_binlog_config);
Review Comment:
[P1] Publish binlog config under the same synchronization contract used by
readers. `sync_meta` runs on the internal-service/background pools and writes
this multi-field object under `_meta_lock`, but `create_rowset_writer` and
`create_transient_rowset_writer` read `TabletMeta::binlog_config()` without
that lock (the getter is explicitly marked `TODO` thread safety). This
unconditional periodic assignment therefore creates a C++ data race during
normal loads. Return a lock-protected copy or atomically publish an immutable
snapshot, and avoid rewriting an unchanged config.
##########
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:
[P1] Serialize or version concurrent binlog-config ALTERs. Each statement
snapshots the complete old config, releases the table lock for this remote
fanout, and only later takes the write lock to journal FE state. Two statements
can therefore complete remotely in A-then-B order but journal in B-then-A order
(or interleave batches), both return success, and leave MetaService/tablets
different from FE while losing one independent property change. Use a per-table
operation version/CAS or serialization that spans remote apply and catalog
commit.
--
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]
