ibessonov commented on code in PR #1872:
URL: https://github.com/apache/ignite-3/pull/1872#discussion_r1165371702
##########
examples/src/main/java/org/apache/ignite/example/storage/StorageEngineExample.java:
##########
@@ -65,14 +65,18 @@ void run() throws SQLException {
//--------------------------------------------------------------------------------------
try (Statement stmt = conn.createStatement()) {
+ stmt.executeUpdate(
+ "CREATE ZONE ACCOUNTS_ZONE "
+ + "ENGINE " + engineName
+ + " WITH DATAREGION='" + dataRegionName + "'"
+ );
stmt.executeUpdate(
"CREATE TABLE ACCOUNTS ( "
+ "ACCOUNT_ID INT PRIMARY KEY,"
+ "FIRST_NAME VARCHAR, "
+ "LAST_NAME VARCHAR, "
+ "BALANCE DOUBLE) "
- + " ENGINE " + engineName
- + " WITH dataRegion='" + dataRegionName + "'"
+ + "WITH PRIMARY_ZONE = 'ACCOUNTS_ZONE'"
Review Comment:
Why is it called a "primary zone"? Is there a non-primary zone?
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/configuration/DistributionZoneConfigurationSchema.java:
##########
@@ -53,6 +56,11 @@ public class DistributionZoneConfigurationSchema {
@Value(hasDefault = true)
public int replicas = DEFAULT_REPLICA_COUNT;
+ /** Data storage configuration. */
+ @KnownDataStorage
Review Comment:
I believe that this validator is obsolete at this point. You removed the
"unknown" thing from the code, right?
##########
modules/distribution-zones/src/testFixtures/java/org/apache/ignite/internal/distributionzones/DistributionZonesTestUtil.java:
##########
@@ -18,29 +18,53 @@
package org.apache.ignite.internal.distributionzones;
import java.util.concurrent.CompletableFuture;
+import java.util.function.Consumer;
import
org.apache.ignite.internal.distributionzones.DistributionZoneConfigurationParameters.Builder;
+import
org.apache.ignite.internal.schema.configuration.storage.DataStorageChange;
/**
* Utils to manage distribution zones inside tests.
*/
public class DistributionZonesTestUtil {
+
/**
* Creates distribution zone.
*
* @param zoneManager Zone manager.
* @param zoneName Zone name.
* @param partitions Zone number of partitions.
* @param replicas Zone number of replicas.
+ * @param dataStorageChangeConsumer Consumer of {@link DataStorageChange},
which sets the right data storage options.
* @return A future, which will be completed, when create operation
finished.
*/
public static CompletableFuture<Integer> createZone(
- DistributionZoneManager zoneManager, String zoneName, int
partitions, int replicas) {
+ DistributionZoneManager zoneManager, String zoneName,
+ int partitions, int replicas, Consumer<DataStorageChange>
dataStorageChangeConsumer) {
var distributionZoneCfgBuilder = new Builder(zoneName)
.replicas(replicas)
.partitions(partitions);
- var distributionZoneCfg = distributionZoneCfgBuilder.build();
- return zoneManager.createZone(distributionZoneCfg).thenApply((v) ->
zoneManager.getZoneId(zoneName));
+ if (dataStorageChangeConsumer != null) {
+ distributionZoneCfgBuilder
+ .dataStorageChangeConsumer(dataStorageChangeConsumer);
+ }
+
+ return
zoneManager.createZone(distributionZoneCfgBuilder.build()).thenApply((v) ->
zoneManager.getZoneId(zoneName));
Review Comment:
Why can't `zoneManager` return a future with `zoneId`?
##########
modules/schema/src/test/java/org/apache/ignite/internal/schema/configuration/KnownDataStorageValidatorTest.java:
##########
@@ -39,6 +40,7 @@
@ExtendWith(ConfigurationExtension.class)
public class KnownDataStorageValidatorTest {
@InjectConfiguration(
+ value = "mock.name = " +
UnknownDataStorageConfigurationSchema.UNKNOWN_DATA_STORAGE,
Review Comment:
Oh, so you didn't remove the unknown storage. Why?
##########
modules/storage-page-memory/src/testFixtures/java/org/apache/ignite/internal/storage/pagememory/TestPersisteStorageConfigurationModule.java:
##########
@@ -0,0 +1,53 @@
+/*
+ * 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.storage.pagememory;
+
+import java.util.Collection;
+import java.util.List;
+import org.apache.ignite.configuration.ConfigurationModule;
+import org.apache.ignite.configuration.RootKey;
+import org.apache.ignite.configuration.annotation.ConfigurationType;
+import
org.apache.ignite.internal.storage.pagememory.configuration.schema.PersistentPageMemoryDataStorageConfigurationSchema;
+
+/**
+ * Configuration module for mock configuration {@link
TestPersistStorageConfigurationSchema}.
+ *
+ * <p>Important: don't use @AutoService here,
+ * because it will produce the collision between the real {@link
PersistentPageMemoryDataStorageConfigurationSchema}
+ * and the {@link TestPersistStorageConfigurationSchema} in the 'test'
configuration of the current module
+ * (because test configuration always depends on testFixtures and main
configuration).
+ */
+public class TestPersisteStorageConfigurationModule implements
ConfigurationModule {
Review Comment:
Typo in the class name
##########
modules/storage-api/build.gradle:
##########
@@ -40,6 +40,9 @@ dependencies {
testImplementation(testFixtures(project(':ignite-schema')))
testImplementation(testFixtures(project(':ignite-core')))
testImplementation(testFixtures(project(':ignite-configuration')))
+ testImplementation(testFixtures(project(':ignite-storage-page-memory'))) {
+ transitive = false
Review Comment:
Can you please explain the necessity of this flag?
And, why do you make this dependency in the first place? "storage-api" tests
should not depend on the implementation, we need to fix it.
##########
modules/placement-driver/src/integrationTest/java/org/apache/ignite/internal/placementdriver/PlacementDriverManagerTest.java:
##########
@@ -103,7 +103,7 @@ public class PlacementDriverManagerTest extends
IgniteAbstractTest {
@InjectConfiguration
private TablesConfiguration tblsCfg;
- @InjectConfiguration
+ @InjectConfiguration()
Review Comment:
```suggestion
@InjectConfiguration
```
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/DistributionZoneConfigurationParameters.java:
##########
@@ -41,6 +43,9 @@ public class DistributionZoneConfigurationParameters {
/** Number of zone partitions. */
private final Integer partitions;
Review Comment:
Why is it `Integer` instead of `int`? What does `null` mean?
##########
modules/placement-driver/src/integrationTest/java/org/apache/ignite/internal/placementdriver/ActiveActorTest.java:
##########
@@ -113,10 +113,10 @@ public class ActiveActorTest extends IgniteAbstractTest {
@Mock
MetaStorageManager msm;
- @InjectConfiguration()
+ @InjectConfiguration
private TablesConfiguration tblsCfg;
- @InjectConfiguration
+ @InjectConfiguration()
Review Comment:
```suggestion
@InjectConfiguration
```
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/sql/DistributionZoneSqlDdlParserTest.java:
##########
@@ -111,19 +112,19 @@ public void createZoneWithOptions() {
assertThatZoneOptionPresent(optList,
IgniteSqlZoneOptionEnum.DATA_NODES_AUTO_ADJUST, 1);
expectUnparsed(createZone, "CREATE ZONE \"TEST_ZONE\" WITH "
- + "REPLICAS = 2, "
- + "PARTITIONS = 3, "
- + "DATA_NODES_FILTER = '(\"US\" || \"EU\") && \"SSD\"', "
- + "AFFINITY_FUNCTION = 'test_Affinity', "
- + "DATA_NODES_AUTO_ADJUST = 1, "
- + "DATA_NODES_AUTO_ADJUST_SCALE_UP = 2, "
- + "DATA_NODES_AUTO_ADJUST_SCALE_DOWN = 3");
+ + "\"REPLICAS\" = 2, "
+ + "\"PARTITIONS\" = 3, "
+ + "\"DATA_NODES_FILTER\" = '(\"US\" || \"EU\") && \"SSD\"', "
+ + "\"AFFINITY_FUNCTION\" = 'test_Affinity', "
+ + "\"DATA_NODES_AUTO_ADJUST\" = 1, "
+ + "\"DATA_NODES_AUTO_ADJUST_SCALE_UP\" = 2, "
+ + "\"DATA_NODES_AUTO_ADJUST_SCALE_DOWN\" = 3");
}
/**
* Parse CREATE ZONE WITH unknown option.
*/
- @Test
+ @Disabled("https://issues.apache.org/jira/browse/IGNITE-19261";)
Review Comment:
Please add a description to the issue that you've created. It's empty now
--
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]
