CRZbulabula commented on code in PR #17425:
URL: https://github.com/apache/iotdb/pull/17425#discussion_r3861141032
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/service/thrift/ConfigNodeRPCServiceProcessor.java:
##########
@@ -537,6 +523,12 @@ public TSStatus setTimePartitionInterval(final
TSetTimePartitionIntervalReq req)
new SetTimePartitionIntervalPlan(req.getDatabase(),
req.getTimePartitionInterval()));
}
+ @Override
+ public TSStatus setTimePartitionOrigin(final TSetTimePartitionOriginReq req)
throws TException {
Review Comment:
Please do not expose a mutable time-partition setter through the RPC layer.
`time_partition_origin` and `time_partition_interval` define the physical
partition key; changing either one remaps timestamps to different
`TTimePartitionSlot` / partition IDs, while existing `DataPartitionTable`
entries and TsFiles are not migrated. These properties should be accepted only
by `CREATE DATABASE`, and the general `ALTER DATABASE` request should reject
them.
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/persistence/schema/ClusterSchemaInfo.java:
##########
@@ -280,11 +284,21 @@ public TSStatus alterDatabase(final DatabaseSchemaPlan
plan) {
currentSchema.isNeedLastCache());
}
+ if (alterSchema.isSetTimePartitionOrigin()) {
+
currentSchema.setTimePartitionOrigin(alterSchema.getTimePartitionOrigin());
Review Comment:
Please remove this ALTER mutation. Updating only the schema and
`TimePartitionUtils` changes the partition function for an existing database
without migrating its data-partition table, region assignments, TsFile
partition IDs, or recovery metadata. The CREATE path should persist either the
explicit value or the resolved global default, and these values must remain
immutable afterwards.
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/manager/schema/ClusterSchemaManager.java:
##########
@@ -485,6 +498,41 @@ public TSStatus setTimePartitionInterval(
}
}
+ public TSStatus setTimePartitionOrigin(SetTimePartitionOriginPlan
setTimePartitionOriginPlan) {
+ final TSStatus validationResult =
+ validateTimePartitionConfig(
+ new TDatabaseSchema(setTimePartitionOriginPlan.getDatabase())
+
.setTimePartitionOrigin(setTimePartitionOriginPlan.getTimePartitionOrigin()));
+ if (validationResult != null) {
+ return validationResult;
+ }
+ try {
+ return getConsensusManager().write(setTimePartitionOriginPlan);
Review Comment:
Please remove this setter path instead of adding DataNode notification.
Because the contract is CREATE-only, origin/interval must not have consensus
setter APIs that can mutate an existing database. Rejecting these fields at the
ALTER boundary also avoids a distributed window where different DataNodes
calculate different partition IDs.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/execution/config/executor/ClusterConfigTaskExecutor.java:
##########
@@ -524,9 +553,15 @@ public SettableFuture<ConfigTaskResult> alterDatabase(
}
future.setException(new IoTDBException(tsStatus));
} else {
+ if (databaseSchema.isSetTimePartitionOrigin()
+ || databaseSchema.isSetTimePartitionInterval()
+ || databaseSchema.isSetNeedLastCache()) {
+ ClusterPartitionFetcher.getInstance().invalidAllCache();
Review Comment:
Local cache invalidation cannot make this ALTER safe: it only refreshes the
DataNode executing the statement, while other DataNodes can keep the old
database configuration and route the same timestamp to a different partition
ID. Please remove the ALTER handling end-to-end; cache population should happen
only after CREATE, using explicit values or resolved global defaults.
--
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]
