korlov42 commented on code in PR #2916: URL: https://github.com/apache/ignite-3/pull/2916#discussion_r1415655309
########## modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/RenameZoneCommand.java: ########## @@ -0,0 +1,134 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.catalog.commands; + +import static org.apache.ignite.internal.catalog.CatalogParamsValidationUtils.validateIdentifier; +import static org.apache.ignite.internal.catalog.CatalogService.DEFAULT_ZONE_NAME; + +import java.util.List; +import java.util.Objects; +import org.apache.ignite.internal.catalog.Catalog; +import org.apache.ignite.internal.catalog.CatalogCommand; +import org.apache.ignite.internal.catalog.CatalogValidationException; +import org.apache.ignite.internal.catalog.descriptors.CatalogZoneDescriptor; +import org.apache.ignite.internal.catalog.storage.AlterZoneEntry; +import org.apache.ignite.internal.catalog.storage.UpdateEntry; +import org.apache.ignite.internal.distributionzones.DistributionZoneAlreadyExistsException; +import org.apache.ignite.internal.distributionzones.DistributionZoneNotFoundException; +import org.apache.ignite.internal.lang.IgniteInternalException; +import org.apache.ignite.lang.ErrorGroups.DistributionZones; + +/** + * A command that renames a zone with specified name. + */ +public class RenameZoneCommand extends AbstractZoneCommand { + /** Returns builder to create a command to rename zone. */ + public static RenameZoneCommandBuilder builder() { + return new RenameZoneCommand.Builder(); + } + + private final String newZoneName; + + /** + * Constructor. + * + * @param zoneName Name of the zone. + * @param newZoneName New name of the zone. + * @throws CatalogValidationException if any of restrictions above is violated. + */ + private RenameZoneCommand(String zoneName, String newZoneName) throws CatalogValidationException { + super(zoneName); + + this.newZoneName = newZoneName; + + validate(); + } + + @Override + public List<UpdateEntry> get(Catalog catalog) { + CatalogZoneDescriptor zone = getZone(catalog, zoneName); + + if (catalog.zone(newZoneName) != null) { + throw new DistributionZoneAlreadyExistsException(newZoneName); + } + + if (zone.name().equals(DEFAULT_ZONE_NAME)) { + throw new IgniteInternalException( + DistributionZones.ZONE_RENAME_ERR, + "Default distribution zone can't be renamed" + ); + } + + CatalogZoneDescriptor descriptor = new CatalogZoneDescriptor( + zone.id(), + newZoneName, + zone.partitions(), + zone.replicas(), + zone.dataNodesAutoAdjust(), + zone.dataNodesAutoAdjustScaleUp(), + zone.dataNodesAutoAdjustScaleDown(), + zone.filter(), + zone.dataStorage() + ); + + return List.of(new AlterZoneEntry(descriptor)); + } + + private static CatalogZoneDescriptor getZone(Catalog catalog, String zoneName) { Review Comment: there is already `org.apache.ignite.internal.catalog.commands.CatalogUtils#zoneOrThrow` ########## modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogManagerValidationTest.java: ########## @@ -43,526 +44,559 @@ public class CatalogManagerValidationTest extends BaseCatalogManagerTest { @Test void testValidateZoneNameOnCreateZone() { - assertThat( - manager.createZone(CreateZoneParams.builder().build()), Review Comment: it's time to spit validation per command. See descendants of `org.apache.ignite.internal.catalog.commands.AbstractCommandValidationTest` ########## modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogManagerImpl.java: ########## @@ -337,97 +322,6 @@ public CompletableFuture<Void> execute(List<CatalogCommand> commands) { return saveUpdateAndWaitForActivation(new BulkUpdateProducer(List.copyOf(commands))); } - @Override - public CompletableFuture<Void> createZone(CreateZoneParams params) { - return saveUpdateAndWaitForActivation(catalog -> { - validateCreateZoneParams(params); - - if (catalog.zone(params.zoneName()) != null) { - throw new DistributionZoneAlreadyExistsException(params.zoneName()); - } - - CatalogZoneDescriptor zone = fromParams(catalog.objectIdGenState(), params); - - return List.of( - new NewZoneEntry(zone), - new ObjectIdGenUpdateEntry(1) - ); - }); - } - - @Override - public CompletableFuture<Void> dropZone(DropZoneParams params) { - return saveUpdateAndWaitForActivation(catalog -> { - validateDropZoneParams(params); - - CatalogZoneDescriptor zone = getZone(catalog, params.zoneName()); - - if (zone.name().equals(DEFAULT_ZONE_NAME)) { - throw new IgniteInternalException( - DistributionZones.ZONE_DROP_ERR, - "Default distribution zone can't be dropped" - ); - } - - catalog.schemas().stream() - .flatMap(s -> Arrays.stream(s.tables())) - .filter(t -> t.zoneId() == zone.id()) - .findAny() - .ifPresent(t -> { - throw new DistributionZoneBindTableException(zone.name(), t.name()); - }); - - return List.of(new DropZoneEntry(zone.id())); - }); - } - - @Override - public CompletableFuture<Void> renameZone(RenameZoneParams params) { - return saveUpdateAndWaitForActivation(catalog -> { - validateRenameZoneParams(params); - - CatalogZoneDescriptor zone = getZone(catalog, params.zoneName()); - - if (catalog.zone(params.newZoneName()) != null) { - throw new DistributionZoneAlreadyExistsException(params.newZoneName()); - } - - if (zone.name().equals(DEFAULT_ZONE_NAME)) { - throw new IgniteInternalException( - DistributionZones.ZONE_RENAME_ERR, - "Default distribution zone can't be renamed" - ); - } - - CatalogZoneDescriptor descriptor = new CatalogZoneDescriptor( - zone.id(), - params.newZoneName(), - zone.partitions(), - zone.replicas(), - zone.dataNodesAutoAdjust(), - zone.dataNodesAutoAdjustScaleUp(), - zone.dataNodesAutoAdjustScaleDown(), - zone.filter(), - zone.dataStorage() - ); - - return List.of(new AlterZoneEntry(descriptor)); - }); - } - - @Override - public CompletableFuture<Void> alterZone(AlterZoneParams params) { - return saveUpdateAndWaitForActivation(catalog -> { - validateAlterZoneParams(params); - - CatalogZoneDescriptor zone = getZone(catalog, params.zoneName()); - - CatalogZoneDescriptor descriptor = fromParamsAndPreviousValue(params, zone); - - return List.of(new AlterZoneEntry(descriptor)); - }); - } - Review Comment: `org.apache.ignite.internal.catalog.CatalogManagerImpl#getZone` can be removed ########## modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/RenameZoneCommand.java: ########## @@ -0,0 +1,134 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.catalog.commands; + +import static org.apache.ignite.internal.catalog.CatalogParamsValidationUtils.validateIdentifier; +import static org.apache.ignite.internal.catalog.CatalogService.DEFAULT_ZONE_NAME; + +import java.util.List; +import java.util.Objects; +import org.apache.ignite.internal.catalog.Catalog; +import org.apache.ignite.internal.catalog.CatalogCommand; +import org.apache.ignite.internal.catalog.CatalogValidationException; +import org.apache.ignite.internal.catalog.descriptors.CatalogZoneDescriptor; +import org.apache.ignite.internal.catalog.storage.AlterZoneEntry; +import org.apache.ignite.internal.catalog.storage.UpdateEntry; +import org.apache.ignite.internal.distributionzones.DistributionZoneAlreadyExistsException; +import org.apache.ignite.internal.distributionzones.DistributionZoneNotFoundException; +import org.apache.ignite.internal.lang.IgniteInternalException; +import org.apache.ignite.lang.ErrorGroups.DistributionZones; + +/** + * A command that renames a zone with specified name. + */ +public class RenameZoneCommand extends AbstractZoneCommand { + /** Returns builder to create a command to rename zone. */ + public static RenameZoneCommandBuilder builder() { + return new RenameZoneCommand.Builder(); + } + + private final String newZoneName; + + /** + * Constructor. + * + * @param zoneName Name of the zone. + * @param newZoneName New name of the zone. + * @throws CatalogValidationException if any of restrictions above is violated. + */ + private RenameZoneCommand(String zoneName, String newZoneName) throws CatalogValidationException { + super(zoneName); + + this.newZoneName = newZoneName; + + validate(); + } + + @Override + public List<UpdateEntry> get(Catalog catalog) { + CatalogZoneDescriptor zone = getZone(catalog, zoneName); + + if (catalog.zone(newZoneName) != null) { + throw new DistributionZoneAlreadyExistsException(newZoneName); + } + + if (zone.name().equals(DEFAULT_ZONE_NAME)) { + throw new IgniteInternalException( Review Comment: the same ########## modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/RenameZoneParams.java: ########## @@ -1,68 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one or more - * contributor license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright ownership. - * The ASF licenses this file to You under the Apache License, Version 2.0 - * (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.apache.ignite.internal.catalog.commands; - -/** - * ALTER ZONE RENAME TO statement. - */ -public class RenameZoneParams extends AbstractZoneCommandParams { Review Comment: why don't you remove rest of the zone-related params? ########## modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/RenameZoneCommand.java: ########## @@ -0,0 +1,134 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.catalog.commands; + +import static org.apache.ignite.internal.catalog.CatalogParamsValidationUtils.validateIdentifier; +import static org.apache.ignite.internal.catalog.CatalogService.DEFAULT_ZONE_NAME; + +import java.util.List; +import java.util.Objects; +import org.apache.ignite.internal.catalog.Catalog; +import org.apache.ignite.internal.catalog.CatalogCommand; +import org.apache.ignite.internal.catalog.CatalogValidationException; +import org.apache.ignite.internal.catalog.descriptors.CatalogZoneDescriptor; +import org.apache.ignite.internal.catalog.storage.AlterZoneEntry; +import org.apache.ignite.internal.catalog.storage.UpdateEntry; +import org.apache.ignite.internal.distributionzones.DistributionZoneAlreadyExistsException; +import org.apache.ignite.internal.distributionzones.DistributionZoneNotFoundException; +import org.apache.ignite.internal.lang.IgniteInternalException; +import org.apache.ignite.lang.ErrorGroups.DistributionZones; + +/** + * A command that renames a zone with specified name. + */ +public class RenameZoneCommand extends AbstractZoneCommand { + /** Returns builder to create a command to rename zone. */ + public static RenameZoneCommandBuilder builder() { + return new RenameZoneCommand.Builder(); + } + + private final String newZoneName; + + /** + * Constructor. + * + * @param zoneName Name of the zone. + * @param newZoneName New name of the zone. + * @throws CatalogValidationException if any of restrictions above is violated. + */ + private RenameZoneCommand(String zoneName, String newZoneName) throws CatalogValidationException { + super(zoneName); + + this.newZoneName = newZoneName; + + validate(); + } + + @Override + public List<UpdateEntry> get(Catalog catalog) { + CatalogZoneDescriptor zone = getZone(catalog, zoneName); + + if (catalog.zone(newZoneName) != null) { + throw new DistributionZoneAlreadyExistsException(newZoneName); Review Comment: Catalog command should throw CatalogValidationException ########## modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/AbstractZoneCommand.java: ########## @@ -0,0 +1,43 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.catalog.commands; + +import static org.apache.ignite.internal.catalog.CatalogParamsValidationUtils.validateIdentifier; + +import org.apache.ignite.internal.catalog.CatalogCommand; +import org.apache.ignite.internal.catalog.CatalogValidationException; + +/** + * Abstract zone-related command. + * + * <p>Every table-related command, disregard it going to create new table or modify existing one, + * should specify name of the table and namespace (schema) where to find existing/put new table. Review Comment: I see this is copy-pasted from table's command... Let's adjust javadoc accordingly ########## modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CatalogUtils.java: ########## @@ -198,6 +199,24 @@ public static CatalogTableColumnDescriptor fromParams(ColumnParams params) { precision, scale, length, defaultValue); } + /** + * Returns appropriate zone descriptor. + * + * @param catalog Catalog descriptor. + * @param zoneName Zone name. + */ + static CatalogZoneDescriptor getZone(Catalog catalog, String zoneName) { Review Comment: there is already `zoneOrThrow` method ########## modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/AbstractZoneCommandBuilder.java: ########## @@ -17,27 +17,17 @@ package org.apache.ignite.internal.catalog.commands; +import org.apache.ignite.internal.catalog.CatalogCommand; + /** - * DROP ZONE statement. + * Abstract builder of zone-related commands. + * + * <p>Every table-related command, disregard it going to create new table or modify existing one, + * should specify name of the table and namespace (schema) where to find existing/put new table. Review Comment: the same as above: javadoc should be adjusted ########## modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/AlterZoneCommandBuilder.java: ########## @@ -0,0 +1,85 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.catalog.commands; + +import org.jetbrains.annotations.Nullable; + +/** + * Builder of a command that changes a particular zone. + * + * <p>A builder is considered to be reusable, thus implementation have + * to make sure invocation of {@link #build()} method doesn't cause any + * side effects on builder's state or any object created by the same builder. + */ +public interface AlterZoneCommandBuilder extends AbstractZoneCommandBuilder<AlterZoneCommandBuilder> { + /** + * Sets the number of partitions. + * + * @param partitions Number of partitions. + * @return This instance. + */ + AlterZoneCommandBuilder partitions(@Nullable Integer partitions); + + /** + * Sets the number of replicas. Review Comment: please add restrictions to the param description to every param of Avery builder (restrictions like `Optional`, or `Should be in range 1..Int.MAX_VALUE`; in other words, anything that is being validated ) ########## modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogParamsValidationUtils.java: ########## @@ -49,95 +44,27 @@ public static void validateIdentifier(@Nullable String identifier, String contex } } - static void validateCreateZoneParams(CreateZoneParams params) { - validateUpdateZoneFieldsParameters( - params.zoneName(), - params.partitions(), - params.replicas(), - params.dataNodesAutoAdjust(), - params.dataNodesAutoAdjustScaleUp(), - params.dataNodesAutoAdjustScaleDown(), - params.filter() - ); - } - - static void validateAlterZoneParams(AlterZoneParams params) { - validateUpdateZoneFieldsParameters( - params.zoneName(), - params.partitions(), - params.replicas(), - params.dataNodesAutoAdjust(), - params.dataNodesAutoAdjustScaleUp(), - params.dataNodesAutoAdjustScaleDown(), - params.filter() - ); - } - - static void validateDropZoneParams(DropZoneParams params) { - validateZoneName(params.zoneName()); - } - - static void validateRenameZoneParams(RenameZoneParams params) { - validateZoneName(params.zoneName()); - validateZoneName(params.newZoneName(), "Missing new zone name"); - } - - private static void validateUpdateZoneFieldsParameters( - String zoneName, - @Nullable Integer partitions, - @Nullable Integer replicas, - @Nullable Integer dataNodesAutoAdjust, - @Nullable Integer dataNodesAutoAdjustScaleUp, - @Nullable Integer dataNodesAutoAdjustScaleDown, - @Nullable String filter - ) { - validateZoneName(zoneName); - - validateZonePartitions(partitions); - validateZoneReplicas(replicas); - - validateZoneDataNodesAutoAdjust(dataNodesAutoAdjust); - validateZoneDataNodesAutoAdjustScaleUp(dataNodesAutoAdjustScaleUp); - validateZoneDataNodesAutoAdjustScaleDown(dataNodesAutoAdjustScaleDown); - - validateZoneDataNodesAutoAdjustParametersCompatibility( - dataNodesAutoAdjust, - dataNodesAutoAdjustScaleUp, - dataNodesAutoAdjustScaleDown - ); - - validateZoneFilter(filter); - } - - private static void validateZoneName(String zoneName) { - validateZoneName(zoneName, "Missing zone name"); - } - - private static void validateZoneName(String zoneName, String errorMessage) { - validateNameField(zoneName, DistributionZones.ZONE_DEFINITION_ERR, errorMessage); - } - - private static void validateZonePartitions(@Nullable Integer partitions) { - validateZoneField(partitions, 1, MAX_PARTITION_COUNT, "Invalid number of partitions"); - } - - private static void validateZoneReplicas(@Nullable Integer replicas) { - validateZoneField(replicas, 1, null, "Invalid number of replicas"); - } - - private static void validateZoneDataNodesAutoAdjust(@Nullable Integer dataNodesAutoAdjust) { - validateZoneField(dataNodesAutoAdjust, 0, null, "Invalid data nodes auto adjust"); - } - - private static void validateZoneDataNodesAutoAdjustScaleUp(@Nullable Integer dataNodesAutoAdjustScaleUp) { - validateZoneField(dataNodesAutoAdjustScaleUp, 0, null, "Invalid data nodes auto adjust scale up"); - } + /** + * Validates correctness of incoming value. + */ + public static void validateField(@Nullable Integer value, int min, @Nullable Integer max, String errorPrefix) { + if (value == null) { + return; + } - private static void validateZoneDataNodesAutoAdjustScaleDown(@Nullable Integer dataNodesAutoAdjustScaleDown) { - validateZoneField(dataNodesAutoAdjustScaleDown, 0, null, "Invalid data nodes auto adjust scale down"); + if (value < min || (max != null && value > max)) { + throw new CatalogValidationException( + DistributionZones.ZONE_DEFINITION_ERR, Review Comment: let's throw code from catalog's error group ()or just Catalog.VALIDATION_ERR. The same for rest of validation exception -- 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]
