ibessonov commented on a change in pull request #754:
URL: https://github.com/apache/ignite-3/pull/754#discussion_r840471167



##########
File path: 
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/DataStorageManager.java
##########
@@ -0,0 +1,116 @@
+/*
+ * 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;
+
+import java.nio.file.Path;
+import java.util.Map;
+import java.util.ServiceLoader;
+import java.util.concurrent.ConcurrentHashMap;
+import org.apache.ignite.configuration.schemas.store.DataStorageConfiguration;
+import org.apache.ignite.internal.configuration.ConfigurationRegistry;
+import org.apache.ignite.internal.manager.IgniteComponent;
+import org.apache.ignite.internal.storage.engine.StorageEngine;
+import org.apache.ignite.internal.storage.engine.StorageEngineFactory;
+import org.apache.ignite.internal.tostring.S;
+import org.apache.ignite.internal.util.IgniteUtils;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Data storage manager.
+ */
+public class DataStorageManager implements IgniteComponent {
+    private final ConfigurationRegistry clusterConfigRegistry;
+
+    private final Path storagePath;
+
+    /** Mapping: {@link StorageEngine#name} -> {@link StorageEngine}. */
+    private final Map<String, StorageEngine> engines = new 
ConcurrentHashMap<>();
+
+    /**
+     * Constructor.
+     *
+     * @param clusterConfigRegistry Register of the (distributed) cluster 
configuration.
+     * @param storagePath Storage path.
+     */
+    public DataStorageManager(
+            ConfigurationRegistry clusterConfigRegistry,
+            Path storagePath
+    ) {
+        this.clusterConfigRegistry = clusterConfigRegistry;
+        this.storagePath = storagePath;
+    }
+
+    /** {@inheritDoc} */
+    @Override
+    public void start() {
+        for (StorageEngineFactory engineFactory : engineFactories()) {
+            StorageEngine engine = 
engineFactory.createEngine(clusterConfigRegistry, storagePath);

Review comment:
       Can't we put them in the map in constructor? I'm not sure honestly, but 
for me it would look better that way

##########
File path: 
modules/api/src/main/java/org/apache/ignite/configuration/schemas/store/DataStorageConfigurationSchema.java
##########
@@ -17,28 +17,18 @@
 
 package org.apache.ignite.configuration.schemas.store;
 
-import org.apache.ignite.configuration.annotation.ConfigValue;
-import org.apache.ignite.configuration.annotation.ConfigurationRoot;
-import org.apache.ignite.configuration.annotation.ConfigurationType;
-import org.apache.ignite.configuration.annotation.Name;
-import org.apache.ignite.configuration.annotation.NamedConfigValue;
-import org.apache.ignite.configuration.validation.ExceptKeys;
+import org.apache.ignite.configuration.annotation.PolymorphicConfig;
+import org.apache.ignite.configuration.annotation.PolymorphicId;
 
 /**
- * Root configuration for data storages.
+ * Configuration schema for data storage.
  */
-@ConfigurationRoot(rootName = "db", type = ConfigurationType.DISTRIBUTED)
+@PolymorphicConfig
 public class DataStorageConfigurationSchema {
-    /** Name of the default data region. */
-    public static final String DEFAULT_DATA_REGION_NAME = "default";
+    /** Default data storage name. */
+    public static final String DEFAULT_DATA_STORAGE_NAME = "rocksdb";

Review comment:
       As far as I see in Andrey Gura's design (we discussed it in person), 
there will be no default in the future, just keep that in mind. No need to 
remove it right now.

##########
File path: 
modules/storage-api/src/test/java/org/apache/ignite/internal/storage/chm/TestConcurrentHashMapPartitionStorage.java
##########
@@ -51,17 +52,30 @@
 /**
  * Storage implementation based on {@link ConcurrentHashMap}.
  */
-public class ConcurrentHashMapPartitionStorage implements PartitionStorage {
+public class TestConcurrentHashMapPartitionStorage implements PartitionStorage 
{

Review comment:
       Why did you rename this class? Just wondering

##########
File path: 
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/PageMemoryDataRegion.java
##########
@@ -48,7 +47,7 @@ public PageMemoryDataRegion(PageMemoryDataRegionConfiguration 
cfg, PageIoRegistr
 
     /** {@inheritDoc} */
     @Override
-    public void stop() {
+    public void stop() throws Exception {

Review comment:
       Does it actually throw any checked exception?

##########
File path: 
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/configuration/schema/RocksDbDataStorageConfigurationSchema.java
##########
@@ -0,0 +1,38 @@
+/*
+ * 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.rocksdb.configuration.schema;
+
+import static 
org.apache.ignite.internal.storage.rocksdb.RocksDbStorageEngine.ENGINE_NAME;
+import static 
org.apache.ignite.internal.storage.rocksdb.configuration.schema.RocksDbStorageEngineConfigurationSchema.DEFAULT_DATA_REGION_NAME;
+
+import org.apache.ignite.configuration.annotation.PolymorphicConfigInstance;
+import org.apache.ignite.configuration.annotation.Value;
+import 
org.apache.ignite.configuration.schemas.store.DataStorageConfigurationSchema;
+import org.apache.ignite.configuration.validation.Immutable;
+import org.apache.ignite.internal.storage.rocksdb.RocksDbStorageEngine;
+
+/**
+ * Data storage configuration for {@link RocksDbStorageEngine}.
+ */
+@PolymorphicConfigInstance(ENGINE_NAME)
+public class RocksDbDataStorageConfigurationSchema extends 
DataStorageConfigurationSchema {
+    /** Data region. */
+    @Immutable

Review comment:
       And..., maybe we will remove regions from RocksDb storages one day and 
it's all irrelevant

##########
File path: 
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/configuration/schema/RocksDbDataStorageConfigurationSchema.java
##########
@@ -0,0 +1,38 @@
+/*
+ * 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.rocksdb.configuration.schema;
+
+import static 
org.apache.ignite.internal.storage.rocksdb.RocksDbStorageEngine.ENGINE_NAME;
+import static 
org.apache.ignite.internal.storage.rocksdb.configuration.schema.RocksDbStorageEngineConfigurationSchema.DEFAULT_DATA_REGION_NAME;
+
+import org.apache.ignite.configuration.annotation.PolymorphicConfigInstance;
+import org.apache.ignite.configuration.annotation.Value;
+import 
org.apache.ignite.configuration.schemas.store.DataStorageConfigurationSchema;
+import org.apache.ignite.configuration.validation.Immutable;
+import org.apache.ignite.internal.storage.rocksdb.RocksDbStorageEngine;
+
+/**
+ * Data storage configuration for {@link RocksDbStorageEngine}.
+ */
+@PolymorphicConfigInstance(ENGINE_NAME)
+public class RocksDbDataStorageConfigurationSchema extends 
DataStorageConfigurationSchema {
+    /** Data region. */
+    @Immutable

Review comment:
       Should this really be immutable? Old implementation allowed changing 
regions...

##########
File path: 
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/RocksDbStorageEngine.java
##########
@@ -36,45 +42,94 @@
  * Storage engine implementation based on RocksDB.
  */
 public class RocksDbStorageEngine implements StorageEngine {
+    /** Engine name. */
+    public static final String ENGINE_NAME = "rocksdb";
+
     static {
         RocksDB.loadLibrary();
     }
 
+    private final RocksDbStorageEngineConfiguration engineConfig;
+
+    private final Path storagePath;
+
+    private final ExecutorService threadPool = Executors.newFixedThreadPool(
+            Runtime.getRuntime().availableProcessors(),
+            new NamedThreadFactory("rocksdb-storage-engine-pool")
+    );
+
+    private final Map<String, RocksDbDataRegion> regions = new 
ConcurrentHashMap<>();
+
     /**
-     * Thread pool to be used to snapshot partitions and maybe some other 
operations.
+     * Constructor.
+     *
+     * @param engineConfig RocksDB storage engine configuration.
+     * @param storagePath Storage path.
      */
-    private final ExecutorService threadPool = Executors.newFixedThreadPool(
-            Runtime.getRuntime().availableProcessors(), new 
NamedThreadFactory("rocksdb-storage-engine-pool"));
+    public RocksDbStorageEngine(RocksDbStorageEngineConfiguration 
engineConfig, Path storagePath) {
+        this.engineConfig = engineConfig;
+        this.storagePath = storagePath;
+    }
 
     /** {@inheritDoc} */
     @Override
-    public void start() {
+    public String name() {
+        return ENGINE_NAME;
     }
 
     /** {@inheritDoc} */
     @Override
-    public void stop() throws StorageException {
-        IgniteUtils.shutdownAndAwaitTermination(threadPool, 10, 
TimeUnit.SECONDS);
+    public void start() throws StorageException {
+        RocksDbDataRegion defaultRegion = new 
RocksDbDataRegion(engineConfig.defaultRegion());
+
+        defaultRegion.start();
+
+        regions.put(DEFAULT_DATA_REGION_NAME, defaultRegion);
+
+        for (String regionName : 
engineConfig.regions().value().namedListKeys()) {
+            RocksDbDataRegion region = new 
RocksDbDataRegion(engineConfig.regions().get(regionName));
+
+            region.start();
+
+            regions.put(regionName, region);
+        }
     }
 
     /** {@inheritDoc} */
     @Override
-    public DataRegion createDataRegion(DataRegionConfiguration regionCfg) {
-        assert regionCfg instanceof RocksDbDataRegionConfiguration : regionCfg;
+    public void stop() throws StorageException {
+        IgniteUtils.shutdownAndAwaitTermination(threadPool, 10, 
TimeUnit.SECONDS);
 
-        return new RocksDbDataRegion((RocksDbDataRegionConfiguration) 
regionCfg);
+        try {
+            IgniteUtils.closeAll(regions.values().stream().map(region -> 
region::stop));
+        } catch (Exception e) {
+            throw new StorageException("Error when stopping regions", e);
+        }
     }
 
     /** {@inheritDoc} */
     @Override
-    public TableStorage createTable(Path tablePath, TableConfiguration 
tableCfg, DataRegion dataRegion) {
-        assert dataRegion instanceof RocksDbDataRegion : dataRegion;
-
-        return new RocksDbTableStorage(
-                tablePath,
-                tableCfg,
-                threadPool,
-                (RocksDbDataRegion) dataRegion
-        );
+    public TableStorage createTable(TableConfiguration tableCfg) throws 
StorageException {
+        TableView tableView = tableCfg.value();
+
+        assert tableView.dataStorage().name().equals(ENGINE_NAME) : 
tableView.dataStorage().name();
+
+        RocksDbDataStorageView dataStorageView = (RocksDbDataStorageView) 
tableView.dataStorage();
+
+        RocksDbDataRegion dataRegion = 
regions.get(dataStorageView.dataRegion());
+
+        if (dataRegion == null) {

Review comment:
       Technically speaking, this should be impossible and hence should be 
replaced with assertion. What do you think?

##########
File path: 
modules/storage-api/src/test/java/org/apache/ignite/internal/storage/chm/TestConcurrentHashMapStorageEngineDistributedConfigurationModule.java
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.chm;
+
+import java.util.Collection;
+import java.util.List;
+import org.apache.ignite.configuration.annotation.ConfigurationType;
+import org.apache.ignite.internal.configuration.ConfigurationModule;
+import 
org.apache.ignite.internal.storage.chm.schema.TestConcurrentHashMapDataStorageConfigurationSchema;
+
+/**
+ * Implementation for {@link TestConcurrentHashMapStorageEngine}.
+ */
+public class TestConcurrentHashMapStorageEngineDistributedConfigurationModule 
implements ConfigurationModule {

Review comment:
       That's mouthful :) I believe the word Distributed can be removed here

##########
File path: 
modules/page-memory/src/integrationTest/java/org/apache/ignite/internal/pagememory/persistence/ItBplusTreePageMemoryImplTest.java
##########
@@ -54,7 +49,7 @@ protected PageMemory createPageMemory() throws Exception {
 
         return new PageMemoryImpl(
                 new UnsafeMemoryProvider(null),
-                (PageMemoryDataRegionConfiguration) 
fixConfiguration(dataRegionCfg),
+                fixConfiguration(dataRegionCfg),

Review comment:
       Do you need to invoke "fixConfiguration" then?

##########
File path: 
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/engine/StorageEngine.java
##########
@@ -27,32 +26,31 @@
  */
 public interface StorageEngine {
     /**
-     * Starts the engine.
+     * Returns the unique name of the storage engine.
+     *
+     * <p>NOTE: Used to map {@link DataStorageConfigurationSchema#name} to 
{@link StorageEngine#name}.

Review comment:
       Why is it a NOTE and not the regular sentence? Is this a common notation 
in Ignite?

##########
File path: 
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/DataStorageManager.java
##########
@@ -0,0 +1,116 @@
+/*
+ * 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;
+
+import java.nio.file.Path;
+import java.util.Map;
+import java.util.ServiceLoader;
+import java.util.concurrent.ConcurrentHashMap;
+import org.apache.ignite.configuration.schemas.store.DataStorageConfiguration;
+import org.apache.ignite.internal.configuration.ConfigurationRegistry;
+import org.apache.ignite.internal.manager.IgniteComponent;
+import org.apache.ignite.internal.storage.engine.StorageEngine;
+import org.apache.ignite.internal.storage.engine.StorageEngineFactory;
+import org.apache.ignite.internal.tostring.S;
+import org.apache.ignite.internal.util.IgniteUtils;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Data storage manager.
+ */
+public class DataStorageManager implements IgniteComponent {
+    private final ConfigurationRegistry clusterConfigRegistry;
+
+    private final Path storagePath;
+
+    /** Mapping: {@link StorageEngine#name} -> {@link StorageEngine}. */
+    private final Map<String, StorageEngine> engines = new 
ConcurrentHashMap<>();
+
+    /**
+     * Constructor.
+     *
+     * @param clusterConfigRegistry Register of the (distributed) cluster 
configuration.
+     * @param storagePath Storage path.
+     */
+    public DataStorageManager(
+            ConfigurationRegistry clusterConfigRegistry,
+            Path storagePath
+    ) {
+        this.clusterConfigRegistry = clusterConfigRegistry;
+        this.storagePath = storagePath;
+    }
+
+    /** {@inheritDoc} */
+    @Override
+    public void start() {
+        for (StorageEngineFactory engineFactory : engineFactories()) {
+            StorageEngine engine = 
engineFactory.createEngine(clusterConfigRegistry, storagePath);
+
+            engine.start();
+
+            String name = engine.name();
+
+            if (engines.containsKey(name)) {
+                throw new StorageException(
+                        "There is already a storage engine with the same name: 
[name=" + name + ", engine=" + engines.get(name) + "]"
+                );
+            } else {
+                engines.put(name, engine);
+            }
+        }
+    }
+
+    /** {@inheritDoc} */
+    @Override
+    public void stop() throws Exception {
+        IgniteUtils.closeAll(engines.values().stream().map(engine -> 
engine::stop));
+    }
+
+    /**
+     * Returns the storage engine factories.
+     *
+     * <p>NOTE: It is recommended to override only in tests.
+     */
+    protected Iterable<StorageEngineFactory> engineFactories() {
+        return ServiceLoader.load(StorageEngineFactory.class);
+    }
+
+    /**
+     * Returns the data storage engine by name.
+     *
+     * @param name Engine name.
+     */
+    public @Nullable StorageEngine engine(String name) {

Review comment:
       I guess that this or the following method is unused, right?

##########
File path: 
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/MockedStructuresTest.java
##########
@@ -178,6 +194,16 @@ void before() throws NodeStoppingException {
             });
         };
 
+        dsm = new DataStorageManager(configRegistry, workDir) {
+            /** {@inheritDoc} */
+            @Override
+            protected Iterable<StorageEngineFactory> engineFactories() {

Review comment:
       This method exists for tests only, right? Doesn't look good, to be honest

##########
File path: 
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/RocksDbStorageEngineFactory.java
##########
@@ -0,0 +1,42 @@
+/*
+ * 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.rocksdb;
+
+import java.nio.file.Path;
+import org.apache.ignite.internal.configuration.ConfigurationRegistry;
+import org.apache.ignite.internal.storage.StorageException;
+import org.apache.ignite.internal.storage.engine.StorageEngine;
+import org.apache.ignite.internal.storage.engine.StorageEngineFactory;
+import 
org.apache.ignite.internal.storage.rocksdb.configuration.schema.RocksDbStorageEngineConfiguration;
+
+/**
+ * Implementation for creating {@link RocksDbStorageEngine}'s.
+ */
+public class RocksDbStorageEngineFactory implements StorageEngineFactory {
+    /** {@inheritDoc} */
+    @Override
+    public StorageEngine createEngine(ConfigurationRegistry configRegistry, 
Path storagePath) throws StorageException {
+        RocksDbStorageEngineConfiguration engineConfig = 
configRegistry.getConfiguration(RocksDbStorageEngineConfiguration.KEY);
+
+        if (engineConfig == null) {

Review comment:
       How's this possible? Should be assertion as well

##########
File path: 
modules/storage-api/src/test/java/org/apache/ignite/internal/storage/ConcurrentHashMapStorageTest.java
##########
@@ -0,0 +1,77 @@
+/*
+ * 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;
+
+import java.util.concurrent.TimeUnit;
+import 
org.apache.ignite.configuration.schemas.table.HashIndexConfigurationSchema;
+import org.apache.ignite.configuration.schemas.table.TableConfiguration;
+import 
org.apache.ignite.internal.configuration.testframework.ConfigurationExtension;
+import 
org.apache.ignite.internal.configuration.testframework.InjectConfiguration;
+import 
org.apache.ignite.internal.storage.chm.TestConcurrentHashMapPartitionStorage;
+import 
org.apache.ignite.internal.storage.chm.TestConcurrentHashMapStorageEngine;
+import 
org.apache.ignite.internal.storage.chm.schema.TestConcurrentHashMapDataStorageChange;
+import 
org.apache.ignite.internal.storage.chm.schema.TestConcurrentHashMapDataStorageConfigurationSchema;
+import 
org.apache.ignite.internal.storage.chm.schema.TestRocksDbDataStorageConfigurationSchema;
+import org.apache.ignite.internal.storage.engine.StorageEngine;
+import org.apache.ignite.internal.storage.engine.TableStorage;
+import org.apache.ignite.internal.util.IgniteUtils;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+/**
+ * Storage test implementation for {@link 
TestConcurrentHashMapPartitionStorage}.
+ */
+@ExtendWith(ConfigurationExtension.class)
+public class ConcurrentHashMapStorageTest extends AbstractPartitionStorageTest 
{

Review comment:
       Don't we already have this test? 
org.apache.ignite.internal.storage.basic.ConcurrentHashMapStorageTest

##########
File path: 
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/PageMemoryStorageEngineFactory.java
##########
@@ -0,0 +1,47 @@
+/*
+ * 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.nio.file.Path;
+import org.apache.ignite.internal.configuration.ConfigurationRegistry;
+import org.apache.ignite.internal.pagememory.io.PageIoRegistry;
+import org.apache.ignite.internal.storage.StorageException;
+import org.apache.ignite.internal.storage.engine.StorageEngine;
+import org.apache.ignite.internal.storage.engine.StorageEngineFactory;
+import 
org.apache.ignite.internal.storage.pagememory.configuration.schema.PageMemoryStorageEngineConfiguration;
+
+/**
+ * Implementation for creating {@link PageMemoryStorageEngine}'s.

Review comment:
       Why the apostrophe, it shouldn't be here

##########
File path: 
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/configuration/schema/PageMemoryStorageEngineConfigurationSchema.java
##########
@@ -0,0 +1,48 @@
+/*
+ * 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.configuration.schema;
+
+import static 
org.apache.ignite.configuration.annotation.ConfigurationType.DISTRIBUTED;
+import static 
org.apache.ignite.internal.storage.pagememory.PageMemoryStorageEngine.ENGINE_NAME;
+
+import org.apache.ignite.configuration.annotation.ConfigValue;
+import org.apache.ignite.configuration.annotation.ConfigurationRoot;
+import org.apache.ignite.configuration.annotation.Name;
+import org.apache.ignite.configuration.annotation.NamedConfigValue;
+import org.apache.ignite.configuration.validation.ExceptKeys;
+import 
org.apache.ignite.internal.pagememory.configuration.schema.PageMemoryDataRegionConfigurationSchema;
+import org.apache.ignite.internal.storage.pagememory.PageMemoryStorageEngine;
+
+/**
+ * Root configuration for {@link PageMemoryStorageEngine}.
+ */
+@ConfigurationRoot(rootName = ENGINE_NAME, type = DISTRIBUTED)

Review comment:
       This connection with engine name is unnecessary, I would avoid it and 
maybe even used a different root name




-- 
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]


Reply via email to