Copilot commented on code in PR #17988:
URL: https://github.com/apache/iotdb/pull/17988#discussion_r3448183665
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/manager/schema/ClusterSchemaManager.java:
##########
@@ -908,14 +942,112 @@ public static TSStatus
enrichDatabaseSchemaWithDefaultProperties(
.FAILED_TO_CREATE_DATABASE_THE_DATAREGIONGROUPNUM_SHOULD_BE_POSITIVE);
}
+ if (databaseSchema.isSetMaxSchemaRegionGroupNum()) {
+ errorResp =
+ validateMaxRegionGroupNumOnCreation(
+ databaseSchema, TConsensusGroupType.SchemaRegion, errorResp);
+ }
+ if (databaseSchema.isSetMaxDataRegionGroupNum()) {
+ errorResp =
+ validateMaxRegionGroupNumOnCreation(
+ databaseSchema, TConsensusGroupType.DataRegion, errorResp);
+ }
+
if (errorResp != null) {
LOGGER.warn(ConfigNodeMessages.EXECUTE_SETDATABASE_WITH_RESULT,
databaseSchema, errorResp);
return errorResp;
}
- // The maxRegionGroupNum is equal to the minRegionGroupNum when initialize
-
databaseSchema.setMaxSchemaRegionGroupNum(databaseSchema.getMinSchemaRegionGroupNum());
-
databaseSchema.setMaxDataRegionGroupNum(databaseSchema.getMinDataRegionGroupNum());
+ if (!databaseSchema.isSetMaxSchemaRegionGroupNum()) {
+
databaseSchema.setMaxSchemaRegionGroupNum(databaseSchema.getMinSchemaRegionGroupNum());
+ }
+ if (!databaseSchema.isSetMaxDataRegionGroupNum()) {
+
databaseSchema.setMaxDataRegionGroupNum(databaseSchema.getMinDataRegionGroupNum());
+ }
+
+ return StatusUtils.OK;
+ }
+
+ private static TSStatus validateMaxRegionGroupNumOnCreation(
+ final TDatabaseSchema databaseSchema,
+ final TConsensusGroupType consensusGroupType,
+ final TSStatus previousError) {
+ final TSStatus status =
+ validateMaxRegionGroupNum(
+ databaseSchema.getName(),
+ consensusGroupType,
+ TConsensusGroupType.SchemaRegion.equals(consensusGroupType)
+ ? databaseSchema.getMaxSchemaRegionGroupNum()
+ : databaseSchema.getMaxDataRegionGroupNum(),
+ true);
+ return status.getCode() == TSStatusCode.SUCCESS_STATUS.getStatusCode() ?
previousError : status;
+ }
+
+ private static TSStatus validateMaxRegionGroupNum(
+ final String database,
+ final TConsensusGroupType consensusGroupType,
+ final int maxRegionGroupNum,
+ final boolean isCreate) {
+ final boolean isSchemaRegion =
TConsensusGroupType.SchemaRegion.equals(consensusGroupType);
+ final RegionGroupExtensionPolicy policy =
+ isSchemaRegion
+ ? CONF.getSchemaRegionGroupExtensionPolicy()
+ : CONF.getDataRegionGroupExtensionPolicy();
+ final String configKey =
+ isSchemaRegion ? "max_schema_region_group_num" :
"max_data_region_group_num";
+ final String fieldName = isSchemaRegion ? "MaxSchemaRegionGroupNum" :
"MaxDataRegionGroupNum";
+
+ if (!policy.equals(RegionGroupExtensionPolicy.CUSTOM)) {
+ return new TSStatus(TSStatusCode.DATABASE_CONFIG_ERROR.getStatusCode())
+ .setMessage(
+ String.format(
+ "Failed to %s database. The %s can only be set when
%s_region_group_extension_policy is CUSTOM.",
+ isCreate ? "create" : "alter", configKey, isSchemaRegion ?
"schema" : "data"));
+ }
+
+ final int defaultRegionGroupNum =
+ isSchemaRegion
+ ? CONF.getDefaultSchemaRegionGroupNumPerDatabase()
+ : CONF.getDefaultDataRegionGroupNumPerDatabase();
+ if (maxRegionGroupNum < defaultRegionGroupNum) {
+ return new TSStatus(TSStatusCode.DATABASE_CONFIG_ERROR.getStatusCode())
+ .setMessage(
+ String.format(
+ "%s should be greater than or equal to default
%sRegionGroupNum: %d.",
+ fieldName, isSchemaRegion ? "Schema" : "Data",
defaultRegionGroupNum));
+ }
+
+ return StatusUtils.OK;
+ }
+
+ private TSStatus validateMaxRegionGroupNumOnAlter(
+ final String database,
+ final TConsensusGroupType consensusGroupType,
+ final int maxRegionGroupNum) {
+ TSStatus status =
+ validateMaxRegionGroupNum(database, consensusGroupType,
maxRegionGroupNum, false);
+ if (status.getCode() != TSStatusCode.SUCCESS_STATUS.getStatusCode()) {
+ return status;
+ }
+
+ final boolean isSchemaRegion =
TConsensusGroupType.SchemaRegion.equals(consensusGroupType);
+ final String fieldName = isSchemaRegion ? "MaxSchemaRegionGroupNum" :
"MaxDataRegionGroupNum";
+
+ final int allocatedRegionGroupCount;
+ try {
+ allocatedRegionGroupCount =
+ getPartitionManager().getRegionGroupCount(database,
consensusGroupType);
+ } catch (final DatabaseNotExistsException e) {
+ return new TSStatus(TSStatusCode.DATABASE_NOT_EXIST.getStatusCode())
+ .setMessage(e.getMessage());
+ }
+ if (maxRegionGroupNum < allocatedRegionGroupCount) {
+ return new TSStatus(TSStatusCode.DATABASE_CONFIG_ERROR.getStatusCode())
+ .setMessage(
+ String.format(
+ "%s should be greater than or equal to allocated
%sRegionGroupNum: %d.",
+ fieldName, isSchemaRegion ? "Schema" : "Data",
allocatedRegionGroupCount));
+ }
return StatusUtils.OK;
Review Comment:
`validateMaxRegionGroupNumOnAlter` checks the new max value against the
allocated RegionGroup count, but it doesn’t ensure `max*RegionGroupNum >=
current min*RegionGroupNum`. Since `ClusterSchemaInfo.alterDatabase` applies
`setMax*RegionGroupNum` directly (without clamping), this can make `max < min`
for existing DBs that already have `min` raised (e.g., created/altered before
this change), leading to inconsistent schema constraints.
##########
integration-test/src/test/java/org/apache/iotdb/db/it/utils/TestUtils.java:
##########
@@ -1216,6 +1216,7 @@ public static boolean tryExecuteNonQueriesWithRetry(
}
}
connectionToUse = null;
+ statement = null;
Review Comment:
On retry after a `SQLException`, the code nulls out `statement` so it can be
recreated, but it never closes the existing `Statement`. This can leak JDBC
resources, especially when `defaultConnection` is provided (since the
connection isn’t closed here).
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/statement/metadata/DatabaseSchemaStatement.java:
##########
@@ -152,10 +152,10 @@ public String toString() {
+ dataReplicationFactor
+ ", timePartitionInterval="
+ timePartitionInterval
- + ", schemaRegionGroupNum="
- + schemaRegionGroupNum
- + ", dataRegionGroupNum="
- + dataRegionGroupNum
+ + ", maxSchemaRegionGroupNum="
+ + maxSchemaRegionGroupNum
+ + ", maxDataRegionGroupNum="
+ + maxDataRegionGroupNum
Review Comment:
The updated `toString()` includes the new `max*RegionGroupNum` fields, but
the prefix/key names are still `SetStorageGroupStatement` / `storageGroupPath`.
This is misleading for a `DatabaseSchemaStatement` and makes logs/debug output
confusing when diagnosing CREATE/ALTER DATABASE behavior.
--
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]
