sashapolo commented on code in PR #3266:
URL: https://github.com/apache/ignite-3/pull/3266#discussion_r1507740999
##########
modules/cluster-management/src/integrationTest/java/org/apache/ignite/internal/cluster/management/topology/ItLogicalTopologyTest.java:
##########
@@ -62,7 +63,7 @@ class ItLogicalTopologyTest extends
ClusterPerTestIntegrationTest {
private static final Map<String, String> NODE_ATTRIBUTES_MAP =
Map.of("region", "US", "storage", "SSD");
- private static final Map<String, String> STORAGE_PROFILES_MAP =
Map.of("lru_rocks", "rocksDb", "segmented_aipersist", "aipersist");
+ private static final List<String> STORAGE_PROFILES_LIST =
List.of("lru_rocks", "segmented_aipersist");
Review Comment:
Should this be an array instead?
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/ddl/CreateZoneCommand.java:
##########
@@ -114,11 +91,11 @@ public void affinity(String affinity) {
}
@Nullable public String nodeFilter() {
- return nodeFiler;
+ return nodeFilter;
}
public void nodeFilter(String nodeFiler) {
Review Comment:
```suggestion
public void nodeFilter(String nodeFilter) {
```
##########
modules/storage-configuration/README.md:
##########
@@ -0,0 +1 @@
+# Storage Configuration
Review Comment:
Do we really need such a readme?)
##########
modules/storage-page-memory/src/test/java/org/apache/ignite/internal/storage/pagememory/PageMemoryTestConstants.java:
##########
@@ -0,0 +1,26 @@
+/*
+ * 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;
+
+/**
+ * Useful constants in page memory tests.
+ */
+public class PageMemoryTestConstants {
+ /** Name of the default data region. */
+ public static final String DEFAULT_DATA_REGION_NAME = "default";
Review Comment:
This constant is duplicated in multiple places, why?
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/configuration/PageMemoryStorageEngineLocalConfigurationModule.java:
##########
@@ -20,21 +20,23 @@
import com.google.auto.service.AutoService;
import java.util.Collection;
import java.util.List;
-import java.util.Set;
import org.apache.ignite.configuration.ConfigurationModule;
-import org.apache.ignite.configuration.RootKey;
+import org.apache.ignite.configuration.SuperRootChange;
import org.apache.ignite.configuration.annotation.ConfigurationType;
-import org.apache.ignite.configuration.validation.Validator;
-import
org.apache.ignite.internal.storage.pagememory.configuration.schema.PersistentPageMemoryDataStorageConfigurationSchema;
-import
org.apache.ignite.internal.storage.pagememory.configuration.schema.PersistentPageMemoryStorageEngineConfiguration;
-import
org.apache.ignite.internal.storage.pagememory.configuration.schema.VolatilePageMemoryDataStorageConfigurationSchema;
-import
org.apache.ignite.internal.storage.pagememory.configuration.schema.VolatilePageMemoryStorageEngineConfiguration;
+import
org.apache.ignite.internal.pagememory.configuration.schema.PersistentPageMemoryProfileChange;
+import
org.apache.ignite.internal.pagememory.configuration.schema.PersistentPageMemoryProfileConfigurationSchema;
+import
org.apache.ignite.internal.pagememory.configuration.schema.VolatilePageMemoryProfileConfigurationSchema;
+import org.apache.ignite.internal.storage.configurations.StorageConfiguration;
+import
org.apache.ignite.internal.storage.pagememory.configuration.schema.PersistentPageMemoryStorageEngineExtensionConfigurationSchema;
+import
org.apache.ignite.internal.storage.pagememory.configuration.schema.VolatilePageMemoryStorageEngineExtensionConfigurationSchema;
/**
* {@link ConfigurationModule} for cluster-wide configuration provided by
ignite-storage-page-memory.
Review Comment:
This is strange: the javadoc says "cluster-wide" configuration while the
class is called "LocalConfigurationModule"
##########
modules/compute/src/integrationTest/java/org/apache/ignite/internal/compute/ItWorkerShutdownTest.java:
##########
@@ -370,7 +371,7 @@ private TestingJobExecution<String>
executeGlobalInteractiveJob(IgniteImpl entry
private void createReplicatedTestTableWithOneRow() {
// Number of replicas == number of nodes and number of partitions ==
1. This gives us the majority on primary replica stop.
// After the primary replica is stopped we still be able to select new
primary replica selected.
- executeSql("CREATE ZONE TEST_ZONE WITH REPLICAS=3, PARTITIONS=1");
+ executeSql("CREATE ZONE TEST_ZONE WITH REPLICAS=3, PARTITIONS=1,
STORAGE_PROFILES='" + DEFAULT_STORAGE_PROFILE + "'");
Review Comment:
Do we still need to specify the default profile?
##########
modules/configuration-api/src/main/java/org/apache/ignite/configuration/ConfigurationModule.java:
##########
@@ -97,7 +97,10 @@ default Collection<Class<?>> polymorphicSchemaExtensions() {
/**
* Patches the provided configuration with dynamic default values. This
method is called
- * only for cluster-wide configuration on cluster initialization.
+ * <ul>
+ * <li>for cluster-wide configuration on cluster initialization.</li>
+ * <li>for node-local configuration on configuration read.</li>
Review Comment:
What do you mean by "configuration read"? Is it the first read? Or is it on
configuration instantiation? Or is it patched on every read?
##########
modules/storage-api/README.md:
##########
@@ -53,4 +53,4 @@ String dataRegion = dataStorageView.dateRegion();
```
-To get the data storage engine, you need to use
`org.apache.ignite.internal.storage.DataStorageManager.engine(org.apache.ignite.configuration.schemas.store.DataStorageConfiguration)`.
\ No newline at end of file
+To get the data storage engine, you need to use
`org.apache.ignite.internal.storage.DataStorageManager.engine(org.apache.ignite.configuration.schemas.store.DataStorageConfiguration)`.
Review Comment:
Looks like this whole document is now obsolete, it still mentions data
regions
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlToCatalogCommandConverter.java:
##########
@@ -84,9 +82,9 @@ static CatalogCommand convert(DropTableCommand cmd) {
}
static CatalogCommand convert(CreateZoneCommand cmd) {
- // TODO: IGNITE-19719 We need to define the default engine differently
and the parameters should depend on the engine
- String engine = Objects.requireNonNullElse(cmd.dataStorage(),
DEFAULT_STORAGE_ENGINE);
- String dataRegion = (String)
cmd.dataStorageOptions().getOrDefault("dataRegion", DEFAULT_DATA_REGION);
+ if (cmd.storageProfiles() == null) {
Review Comment:
Don't we check this stuff inside the command itself?
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/DataStorageModules.java:
##########
@@ -74,6 +65,7 @@ public DataStorageModules(Iterable<DataStorageModule>
dataStorageModules) {
assert !modules.isEmpty();
this.modules = modules;
+
Review Comment:
Looks like an accidental change
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogStorageProfilesDescriptor.java:
##########
@@ -0,0 +1,89 @@
+/*
+ * 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 static
org.apache.ignite.internal.catalog.storage.serialization.CatalogSerializationUtils.readList;
+import static
org.apache.ignite.internal.catalog.storage.serialization.CatalogSerializationUtils.writeList;
+
+import java.io.IOException;
+import java.util.List;
+import
org.apache.ignite.internal.catalog.storage.serialization.CatalogObjectSerializer;
+import org.apache.ignite.internal.tostring.S;
+import org.apache.ignite.internal.util.io.IgniteDataInput;
+import org.apache.ignite.internal.util.io.IgniteDataOutput;
+
+/**
+ * Storage profiles descriptor.
+ */
+public class CatalogStorageProfilesDescriptor {
+ public static CatalogObjectSerializer<CatalogStorageProfilesDescriptor>
SERIALIZER = new StorageProfilesDescriptorSerializer();
+ private final List<CatalogStorageProfileDescriptor> storageProfiles;
Review Comment:
Missing new line
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/DataStorageManager.java:
##########
@@ -39,21 +35,35 @@ public class DataStorageManager implements IgniteComponent {
/** Mapping: {@link DataStorageModule#name} -> {@link StorageEngine}. */
private final Map<String, StorageEngine> engines;
+ /** Mapping: {@link StorageProfileConfiguration#name()} -> {@link
StorageEngine#name()}. */
+ private Map<String, String> profilesToEngines;
+
+ /** Storage configuration. **/
+ private StorageConfiguration storageConfiguration;
+
/**
* Constructor.
*
* @param engines Storage engines unique by {@link DataStorageModule#name
name}.
+ * @param storageConfiguration Storage configuration. Needed to extract
the storage profiles configurations after start.
*/
- public DataStorageManager(Map<String, StorageEngine> engines) {
+ public DataStorageManager(Map<String, StorageEngine> engines,
StorageConfiguration storageConfiguration) {
assert !engines.isEmpty();
this.engines = engines;
+ this.storageConfiguration = storageConfiguration;
}
@Override
public CompletableFuture<Void> start() throws StorageException {
engines.values().forEach(StorageEngine::start);
+ profilesToEngines = new HashMap<>();
+
+ storageConfiguration.value().profiles().forEach(profile -> {
Review Comment:
This looks strange, can we simply collect the stream into a map?
##########
modules/storage-page-memory/src/test/java/org/apache/ignite/internal/storage/pagememory/PersistentPageMemoryMvTableStorageTest.java:
##########
@@ -52,13 +53,16 @@ public class PersistentPageMemoryMvTableStorageTest extends
AbstractMvTableStora
@BeforeEach
void setUp(
@WorkDirectory Path workDir,
- @InjectConfiguration
PersistentPageMemoryStorageEngineConfiguration engineConfig
+ @InjectConfiguration
PersistentPageMemoryStorageEngineConfiguration engineConfig,
+ @InjectConfiguration("mock.profiles.default = {engine =
\"aipersist\"}")
Review Comment:
You can simply write `"mock.profiles.default.engine = aipersist"`
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/NodeWithAttributes.java:
##########
@@ -30,11 +31,33 @@ public class NodeWithAttributes implements Serializable {
private final Node node;
- private final Map<String, String> nodeAttributes;
+ private final Map<String, String> userAttributes;
- public NodeWithAttributes(String nodeName, String nodeId, Map<String,
String> nodeAttributes) {
+ private final List<String> storageProfiles;
+
+ /**
+ * Constructor.
+ *
+ * @param nodeName Node name.
+ * @param nodeId Node consistent identifier.
+ * @param userAttributes Key value map of user's node's attributes.
+ */
+ public NodeWithAttributes(String nodeName, String nodeId, Map<String,
String> userAttributes) {
+ this(nodeName, nodeId, userAttributes, List.of());
+ }
+
+ /**
+ * Constructor.
+ *
+ * @param nodeName Node name.
+ * @param nodeId Node consistent identifier.
+ * @param userAttributes Key value map of user's node's attributes.
+ * @param storageProfiles List of supported storage profiles on the node.
+ */
+ public NodeWithAttributes(String nodeName, String nodeId, Map<String,
String> userAttributes, List<String> storageProfiles) {
this.node = new Node(nodeName, nodeId);
- this.nodeAttributes = nodeAttributes;
+ this.userAttributes = userAttributes == null ? Map.of() :
userAttributes;
Review Comment:
Please add corresponding nullability annotations
--
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]
