CRZbulabula commented on code in PR #17988:
URL: https://github.com/apache/iotdb/pull/17988#discussion_r3448168276
##########
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,
Review Comment:
**Unused parameter.** The `database` parameter of
`validateMaxRegionGroupNum` is never referenced in the method body (it reads
policy/default/field-name purely from `consensusGroupType` and `CONF`).
Likewise `validateMaxRegionGroupNumOnCreation` only forwards
`databaseSchema.getName()` into it. Either drop the parameter, or use it in the
error message for clearer diagnostics.
##########
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:
**Unrelated drive-by change.** This `statement = null;` (and the PR's head
commit "The old statement shall be reclaimed in the same manner as the original
connection") is a statement/connection-lifecycle fix that is unrelated to the
`MAX_*_REGION_GROUP_NUM` feature this PR is about. It looks like a legitimate
resource-cleanup fix, but please either split it into its own PR or call it out
in the PR description so reviewers and `git blame` aren't confused about why
it's here.
##########
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;
Review Comment:
**Error-aggregation pattern is replace, not accumulate.**
`validateMaxRegionGroupNumOnCreation` returns `previousError` on success and
`status` on failure, and the two call sites chain it as `errorResp =
validateMaxRegionGroupNumOnCreation(schema, SchemaRegion, errorResp)` then
again for `DataRegion`. If the SchemaRegion validation already produced a
non-null error and the DataRegion validation also fails, the SchemaRegion error
is silently overwritten (DataRegion wins). With current inputs only one error
can occur so it's harmless today, but the pattern reads as if it accumulates
when it actually replaces. The alter path already does the clearer `if (status
!= OK) return status;` per dimension — applying the same shape here on create
would avoid the subtlety.
--
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]
