[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2094: IGNITE-18537 Add add/drop distribution zone DDL commands

["korlov42 (via GitHub)" <gi...@apache.org>]

korlov42 commented on code in PR #2094:
URL: https://github.com/apache/ignite-3/pull/2094#discussion_r1223090021


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/Catalog.java:
##########
@@ -38,42 +39,49 @@ public class Catalog {
     private final int objectIdGen;
     private final long activationTimestamp;
     private final Map<String, CatalogSchemaDescriptor> schemas;
+    private final Map<String, CatalogZoneDescriptor> zones;
 
     @IgniteToStringExclude
     private final Map<Integer, CatalogTableDescriptor> tablesMap;
     @IgniteToStringExclude
     private final Map<Integer, CatalogIndexDescriptor> indexesMap;
+    @IgniteToStringExclude
+    private final Map<Integer, CatalogZoneDescriptor> zonesMap;
 
     /**
      * Constructor.
      *
      * @param version A version of the catalog.
      * @param activationTimestamp A timestamp when this version becomes active (i.e. available for use).
-     * @param objectIdGen Current state of identifier generator. This value should be used to assign an
-     *      id to a new object in the next version of the catalog.
-     * @param descriptors Enumeration of schemas available in the current version of catalog.
+     * @param objectIdGen Current state of identifier generator. This value should be used to assign an id to a new object in the
+     *         next version of the catalog.
+     * @param zones Distribution zones descriptors.
+     * @param schemas Enumeration of schemas available in the current version of catalog.
      */
     public Catalog(
             int version,
             long activationTimestamp,
             int objectIdGen,
-            CatalogSchemaDescriptor... descriptors
+            Collection<CatalogZoneDescriptor> zones,
+            CatalogSchemaDescriptor... schemas
     ) {
         this.version = version;
         this.activationTimestamp = activationTimestamp;
         this.objectIdGen = objectIdGen;
 
-        Objects.requireNonNull(descriptors, "schemas");
+        Objects.requireNonNull(schemas, "schemas");
 
-        assert descriptors.length > 0 : "No schemas found";
-        assert Arrays.stream(descriptors).allMatch(t -> t.version() == version) : "Invalid schema version";
+        assert schemas.length > 0 : "No schemas found";
+        assert Arrays.stream(schemas).allMatch(t -> t.version() == version) : "Invalid schema version";

Review Comment:
   I think, both assertions can be simply removed



##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlToCatalogCommandConverter.java:
##########
@@ -72,6 +80,47 @@ static DropTableParams convert(DropTableCommand cmd) {
                 .build();
     }
 
+    static CreateZoneParams convert(CreateZoneCommand cmd) {
+        return CreateZoneParams.builder()
+                .zoneName(cmd.zoneName())
+                .partitions(cmd.partitions())
+                .replicas(cmd.replicas())

Review Comment:
   seems you have missed the filter



##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/Catalog.java:
##########
@@ -38,42 +39,49 @@ public class Catalog {
     private final int objectIdGen;
     private final long activationTimestamp;
     private final Map<String, CatalogSchemaDescriptor> schemas;
+    private final Map<String, CatalogZoneDescriptor> zones;
 
     @IgniteToStringExclude
     private final Map<Integer, CatalogTableDescriptor> tablesMap;
     @IgniteToStringExclude
     private final Map<Integer, CatalogIndexDescriptor> indexesMap;
+    @IgniteToStringExclude
+    private final Map<Integer, CatalogZoneDescriptor> zonesMap;

Review Comment:
   I would rename all those maps as <zones>ById and <zones>ByName.
   
   Besides, what do you think about using Int2ObjectMap?



##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogSchemaDescriptor.java:
##########
@@ -87,8 +97,21 @@ private void readObject(ObjectInputStream in) throws IOException, ClassNotFoundE
     }
 
     private void rebuildMaps() {
-        tablesMap = Arrays.stream(tables).collect(Collectors.toMap(CatalogObjectDescriptor::name, Function.identity()));
-        indexesMap = Arrays.stream(indexes).collect(Collectors.toMap(CatalogObjectDescriptor::name, Function.identity()));
+        tablesMap = Arrays.stream(tables).collect(Collectors.toUnmodifiableMap(CatalogObjectDescriptor::name, Function.identity()));
+        indexesMap = Arrays.stream(indexes).collect(Collectors.toUnmodifiableMap(CatalogObjectDescriptor::name, Function.identity()));
+    }
+
+    /** Creates new schema descriptor with new version. */
+    public CatalogSchemaDescriptor copy(int version) {

Review Comment:
   well, schema should not have any version. I think, it's better to remove it at all



##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CreateZoneParams.java:
##########
@@ -0,0 +1,191 @@
+/*
+ * 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 java.util.Objects;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * CREATE ZONE statement.
+ */
+public class CreateZoneParams extends AbstractZoneCommandParams {
+    /** Creates parameters builder. */
+    public static Builder builder() {
+        return new Builder();
+    }
+
+    /** Default number of zone partitions. */
+    public static final int DEFAULT_PARTITION_COUNT = 25;
+    /** Default number of zone replicas. */
+    public static final int DEFAULT_REPLICA_COUNT = 1;
+    /** Default infinite value for the distribution zones' timers. */
+    public static final int INFINITE_TIMER_VALUE = Integer.MAX_VALUE;
+
+    /** Amount of zone partitions. */
+    protected int partitions = DEFAULT_PARTITION_COUNT;
+
+    /** Amount of zone partition replicas. */
+    protected int replicas = DEFAULT_REPLICA_COUNT;
+
+    /** Data nodes auto adjust timeout. */
+    protected int dataNodesAutoAdjust = INFINITE_TIMER_VALUE;
+
+    /** Data nodes auto adjust scale up timeout. */
+    protected int dataNodesAutoAdjustScaleUp = INFINITE_TIMER_VALUE;
+
+    /** Data nodes auto adjust scale down timeout. */
+    protected int dataNodesAutoAdjustScaleDown = INFINITE_TIMER_VALUE;
+
+    /**
+     * Default filter value for a distribution zone,
+     * which is a {@link com.jayway.jsonpath.JsonPath} expression for including all attributes of nodes.
+     */
+    public static final String DEFAULT_FILTER = "$..*";

Review Comment:
   "$.+" ??



##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogZoneDescriptor.java:
##########
@@ -0,0 +1,133 @@
+/*
+ * 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.descriptors;
+
+import org.apache.ignite.internal.tostring.S;
+
+/**
+ * Distribution zone descriptor base class.
+ */
+public class CatalogZoneDescriptor extends CatalogObjectDescriptor {
+    private static final long serialVersionUID = 1093607327002694066L;
+
+    /** Amount of zone partitions. */
+    private final int partitions;
+
+    /** Amount of zone replicas. */
+    private final int replicas;
+
+    /** Data nodes auto adjust timeout. */
+    private final int dataNodesAutoAdjust;
+
+    /** Data nodes auto adjust scale up timeout. */
+    private final int dataNodesAutoAdjustScaleUp;
+
+    /** Data nodes auto adjust scale down timeout. */
+    private final int dataNodesAutoAdjustScaleDown;
+
+    /** Nodes filer. */
+    private final String filter;
+
+    /**
+     * Constructs a distribution zone descriptor.
+     *
+     * @param id Id of the distribution zone.
+     * @param name Name of the zone.
+     * @param partitions Amount of partitions in distributions zone.
+     * @param replicas Amount of partition replicas.
+     * @param dataNodesAutoAdjust Data nodes auto adjust timeout.
+     * @param dataNodesAutoAdjustScaleUp Data nodes auto adjust scale up timeout.
+     * @param dataNodesAutoAdjustScaleDown Data nodes auto adjust scale down timeout.
+     * @param filter Nodes filter.
+     */
+    public CatalogZoneDescriptor(
+            int id,
+            String name,
+            int partitions,
+            int replicas,
+            int dataNodesAutoAdjust,

Review Comment:
   do we really need `dataNodesAutoAdjust` field?



##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CreateZoneParams.java:
##########
@@ -0,0 +1,191 @@
+/*
+ * 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 java.util.Objects;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * CREATE ZONE statement.
+ */
+public class CreateZoneParams extends AbstractZoneCommandParams {
+    /** Creates parameters builder. */
+    public static Builder builder() {
+        return new Builder();
+    }
+
+    /** Default number of zone partitions. */
+    public static final int DEFAULT_PARTITION_COUNT = 25;
+    /** Default number of zone replicas. */
+    public static final int DEFAULT_REPLICA_COUNT = 1;
+    /** Default infinite value for the distribution zones' timers. */
+    public static final int INFINITE_TIMER_VALUE = Integer.MAX_VALUE;
+
+    /** Amount of zone partitions. */
+    protected int partitions = DEFAULT_PARTITION_COUNT;
+
+    /** Amount of zone partition replicas. */
+    protected int replicas = DEFAULT_REPLICA_COUNT;
+
+    /** Data nodes auto adjust timeout. */
+    protected int dataNodesAutoAdjust = INFINITE_TIMER_VALUE;
+
+    /** Data nodes auto adjust scale up timeout. */
+    protected int dataNodesAutoAdjustScaleUp = INFINITE_TIMER_VALUE;
+
+    /** Data nodes auto adjust scale down timeout. */
+    protected int dataNodesAutoAdjustScaleDown = INFINITE_TIMER_VALUE;

Review Comment:
   why all those fields have modifier `protected`?



-- 
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: notifications-unsubscribe@ignite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org