ibessonov commented on code in PR #763:
URL: https://github.com/apache/ignite-3/pull/763#discussion_r845850923
##########
modules/api/src/main/java/org/apache/ignite/configuration/schemas/store/ExistDataStorage.java:
##########
@@ -15,16 +15,18 @@
* limitations under the License.
*/
-package org.apache.ignite.internal.schema.configuration.schema;
+package org.apache.ignite.configuration.schemas.store;
-import static
org.apache.ignite.configuration.schemas.store.DataStorageConfigurationSchema.DEFAULT_DATA_STORAGE_NAME;
-
-import org.apache.ignite.configuration.annotation.PolymorphicConfigInstance;
-import
org.apache.ignite.configuration.schemas.store.DataStorageConfigurationSchema;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
/**
- * Test RocksDB data storage configuration schema for tables.
+ * An annotation to check that the value is equal to the {@link
DataStorageConfigurationSchema#name name} of one of the {@link
+ * DataStorageConfigurationSchema data storages}.
*/
-@PolymorphicConfigInstance(DEFAULT_DATA_STORAGE_NAME)
-public class TestRocksDbDataStorageConfigurationSchema extends
DataStorageConfigurationSchema {
+@Target(ElementType.FIELD)
+@Retention(RetentionPolicy.RUNTIME)
+public @interface ExistDataStorage {
Review Comment:
Should this be "Existing"?
##########
modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/Processor.java:
##########
@@ -487,13 +487,22 @@ private void createPojoBindings(
TypeVariableName typeVariable = TypeVariableName.get("T",
changeClsName, POLYMORPHIC_CHANGE_CLASSNAME);
// Method like: <T extends SimpleChange> T convert(Class<T>
changeClass);
- MethodSpec.Builder convertMtdBuilder =
MethodSpec.methodBuilder("convert")
+ MethodSpec.Builder convertByChangeClassMtdBuilder =
MethodSpec.methodBuilder("convert")
.addModifiers(PUBLIC, ABSTRACT)
.addTypeVariable(typeVariable)
.addParameter(parameterType, "changeClass")
.returns(TypeVariableName.get("T"));
- changeClsBuilder.addMethod(convertMtdBuilder.build());
+ changeClsBuilder.addMethod(convertByChangeClassMtdBuilder.build());
+
+ // Method like: <T extends SimpleChange> T convert(String
polymorphicId);
Review Comment:
I'm sorry, this signature makes no sense. What's supposed to be the result
of the method?
First of all, you forgot to mention "& PolymorphicInstance" here and above.
Second, user doesn't expect any specific subclass, given that they only know
string representation of the type. And base "change" class itself does not
extend "PolymorphicInstance".
##########
modules/api/src/main/java/org/apache/ignite/configuration/schemas/store/KnownDataStorage.java:
##########
@@ -15,16 +15,21 @@
* limitations under the License.
*/
-package org.apache.ignite.internal.storage.pagememory.configuration.schema;
+package org.apache.ignite.configuration.schemas.store;
-import static
org.apache.ignite.configuration.schemas.store.DataStorageConfigurationSchema.DEFAULT_DATA_STORAGE_NAME;
-
-import org.apache.ignite.configuration.annotation.PolymorphicConfigInstance;
-import
org.apache.ignite.configuration.schemas.store.DataStorageConfigurationSchema;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+import org.apache.ignite.configuration.annotation.ConfigValue;
/**
- * Test RocksDB data storage configuration schema for tables.
+ * An annotation to check that the {@link DataStorageConfigurationSchema data
storage} is known, i.e. his {@link
Review Comment:
"its", not "his"
##########
modules/api/src/main/java/org/apache/ignite/configuration/schemas/store/DataStorageConfigurationSchema.java:
##########
@@ -19,18 +19,16 @@
import org.apache.ignite.configuration.annotation.PolymorphicConfig;
import org.apache.ignite.configuration.annotation.PolymorphicId;
-import org.apache.ignite.configuration.validation.Immutable;
/**
* Configuration schema for data storage.
*/
@PolymorphicConfig
public class DataStorageConfigurationSchema {
/** Default data storage name. */
- public static final String DEFAULT_DATA_STORAGE_NAME = "rocksdb";
+ public static final String UNKNOWN_DATA_STORAGE = "unknown";
Review Comment:
I believe that schema for unknown storage is located somewhere in the same
module. Can this constant be closer to the implementation? Currently it looks
out of place
##########
modules/schema/src/test/java/org/apache/ignite/internal/schema/configuration/KnownDataStorageValidatorTest.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.schema.configuration;
+
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static
org.apache.ignite.internal.configuration.validation.TestValidationUtil.mockValidationContext;
+import static
org.apache.ignite.internal.configuration.validation.TestValidationUtil.validate;
+import static org.mockito.Mockito.mock;
+
+import org.apache.ignite.configuration.schemas.store.DataStorageConfiguration;
+import org.apache.ignite.configuration.schemas.store.DataStorageView;
+import org.apache.ignite.configuration.schemas.store.KnownDataStorage;
+import
org.apache.ignite.configuration.schemas.store.UnknownDataStorageConfigurationSchema;
+import
org.apache.ignite.internal.configuration.testframework.ConfigurationExtension;
+import
org.apache.ignite.internal.configuration.testframework.InjectConfiguration;
+import
org.apache.ignite.internal.schema.configuration.schema.TestDataStorageChange;
+import
org.apache.ignite.internal.schema.configuration.schema.TestDataStorageConfigurationSchema;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+/**
+ * For {@link KnownDataStorageValidator} testing.
+ */
+@ExtendWith(ConfigurationExtension.class)
+public class KnownDataStorageValidatorTest {
+ @InjectConfiguration(
+ polymorphicExtensions =
{UnknownDataStorageConfigurationSchema.class,
TestDataStorageConfigurationSchema.class}
+ )
+ private DataStorageConfiguration dataStorageConfig;
+
+ @Test
+ void testFailValidation() {
+ KnownDataStorage annotation = mock(KnownDataStorage.class);
+
+ DataStorageView value = dataStorageConfig.value();
+
+ validate(KnownDataStorageValidator.INSTANCE, annotation,
mockValidationContext(value, value), "Data storage cannot be 'unknown'");
+ }
+
+ @Test
+ void testSuccessValidation() throws Exception {
+ KnownDataStorage annotation = mock(KnownDataStorage.class);
+
+ dataStorageConfig.change(c ->
c.convert(TestDataStorageChange.class)).get(1, SECONDS);
+
+ DataStorageView value = dataStorageConfig.value();
+
+ validate(KnownDataStorageValidator.INSTANCE, annotation,
mockValidationContext(value, value), null);
Review Comment:
I see no test where old value and new value would differ
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/CoreDistributedConfigurationModule.java:
##########
@@ -52,7 +63,18 @@ public ConfigurationType type() {
return List.of(
HashIndexConfigurationSchema.class,
SortedIndexConfigurationSchema.class,
- PartialIndexConfigurationSchema.class
+ PartialIndexConfigurationSchema.class,
+ UnknownDataStorageConfigurationSchema.class
+ );
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public Map<Class<? extends Annotation>, Set<Validator<? extends
Annotation, ?>>> validators() {
+ return Map.of(
+ KnownDataStorage.class,
Set.of(KnownDataStorageValidator.INSTANCE),
+ TableValidator.class, Set.of(TableValidatorImpl.INSTANCE),
Review Comment:
Wait a minute, why did you add TableValidatorImpl? Isn't it supposed to be
in another module already?
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/configuration/ExistDataStorageValidator.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * 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.configuration;
+
+import static java.util.stream.Collectors.toUnmodifiableSet;
+import static
org.apache.ignite.configuration.schemas.store.DataStorageConfigurationSchema.UNKNOWN_DATA_STORAGE;
+
+import java.util.Set;
+import java.util.stream.Stream;
+import java.util.stream.StreamSupport;
+import org.apache.ignite.configuration.schemas.store.ExistDataStorage;
+import org.apache.ignite.configuration.validation.ValidationContext;
+import org.apache.ignite.configuration.validation.ValidationIssue;
+import org.apache.ignite.configuration.validation.Validator;
+import org.apache.ignite.internal.storage.engine.StorageEngineFactory;
+
+/**
+ * Implementing a validator for {@link ExistDataStorage}.
+ */
+public class ExistDataStorageValidator implements Validator<ExistDataStorage,
String> {
+ private final Set<String> dataStorages;
+
+ /**
+ * Constructor.
+ *
+ * @param engineFactories Storage engine factories.
+ */
+ public ExistDataStorageValidator(Iterable<StorageEngineFactory>
engineFactories) {
+ dataStorages = Stream.concat(
+ Stream.of(UNKNOWN_DATA_STORAGE),
+ StreamSupport.stream(engineFactories.spliterator(),
false).map(StorageEngineFactory::name)
+ ).collect(toUnmodifiableSet());
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public void validate(ExistDataStorage annotation,
ValidationContext<String> ctx) {
+ String newValue = ctx.getNewValue();
+
+ if (!dataStorages.contains(newValue)) {
+ ctx.addIssue(new ValidationIssue(String.format(
+ "Non-existent data storage [dataStorage=%s,
existDataStorages=%s, key=%s]",
Review Comment:
If these messages become public one day, I'd revisit them in the future. I'm
not asking to change them yet. Although "existDataStorages" makes me somewhat
irritated
##########
modules/storage-api/src/test/java/org/apache/ignite/internal/storage/DataStorageManagerTest.java:
##########
@@ -45,27 +54,85 @@
void testStorageEngineDuplicate() {
String sameName = UUID.randomUUID().toString();
- StorageException exception = assertThrows(
- StorageException.class,
- () -> new
DataStorageManager(Mockito.mock(ConfigurationRegistry.class), workDir) {
- /** {@inheritDoc} */
- @Override
- protected Iterable<StorageEngineFactory> engineFactories()
{
- return List.of(
- (configRegistry, storagePath) ->
createMockedStorageEngine(sameName),
- (configRegistry, storagePath) ->
createMockedStorageEngine(sameName)
- );
- }
- });
+ IllegalStateException exception = assertThrows(
+ IllegalStateException.class,
+ () -> new DataStorageManager(
+ mock(ConfigurationRegistry.class),
+ workDir,
+ List.of(
+ createMockedStorageEngineFactory(sameName),
+ createMockedStorageEngineFactory(sameName)
+ )
+ ));
assertThat(exception.getMessage(), Matchers.startsWith("Duplicate
key"));
}
- private StorageEngine createMockedStorageEngine(String name) {
- StorageEngine mocked = Mockito.mock(StorageEngine.class);
+ @Test
+ void testDefaultTableDataStorageConsumerSingleStorageEngine() {
+ String engineName = UUID.randomUUID().toString();
+
+ DataStorageManager dataStorageManager = new DataStorageManager(
+ mock(ConfigurationRegistry.class),
+ workDir,
+ List.of(createMockedStorageEngineFactory(engineName))
+ );
+
+ checkDefaultTableDataStorageConsumer(dataStorageManager, engineName,
engineName);
+
+ checkDefaultTableDataStorageConsumer(dataStorageManager,
UNKNOWN_DATA_STORAGE, engineName);
+ }
+
+ @Test
+ void testDefaultTableDataStorageConsumerMultipleStorageEngines() {
+ String engineName1 = UUID.randomUUID().toString();
+ String engineName2 = UUID.randomUUID().toString();
+
+ DataStorageManager dataStorageManager = new DataStorageManager(
+ mock(ConfigurationRegistry.class),
+ workDir,
+ List.of(
+ createMockedStorageEngineFactory(engineName1),
+ createMockedStorageEngineFactory(engineName2)
+ )
+ );
+
+ checkDefaultTableDataStorageConsumer(dataStorageManager, engineName1,
engineName1);
+
+ checkDefaultTableDataStorageConsumer(dataStorageManager, engineName2,
engineName2);
+
+ checkDefaultTableDataStorageConsumer(dataStorageManager,
UNKNOWN_DATA_STORAGE, null);
+ }
+
+ private static void checkDefaultTableDataStorageConsumer(
Review Comment:
It would be nice to have a comment - what do we check here? Mockito-based
tests are notoriously hard to read
##########
modules/table/src/test/java/org/apache/ignite/internal/table/TableManagerTest.java:
##########
@@ -600,16 +615,32 @@ private DataStorageManager createDataStorageManager(
Path storagePath,
RocksDbStorageEngineConfiguration config
) {
- DataStorageManager manager = new DataStorageManager(registry,
storagePath) {
- /** {@inheritDoc} */
- @Override
- protected Iterable<StorageEngineFactory> engineFactories() {
- return List.of((registry1, storagePath1) -> new
RocksDbStorageEngine(config, storagePath1));
- }
- };
+ DataStorageManager manager = new DataStorageManager(
+ registry,
+ storagePath,
+ List.of(new StorageEngineFactory() {
Review Comment:
As it was last time, can we avoid anonymous class here?
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -738,23 +738,27 @@ public Table createTable(String name,
Consumer<TableChange> tableInitChange) {
if (tbl != null) {
tblFut.completeExceptionally(new
TableAlreadyExistsException(name));
} else {
- tablesCfg.tables().change(change -> {
- if (change.get(name) != null) {
+ tablesCfg.change(tablesChange ->
tablesChange.changeTables(tablesListChange -> {
+ if (tablesListChange.get(name) != null) {
throw new TableAlreadyExistsException(name);
}
- change.create(name, (ch) -> {
- tableInitChange.accept(ch);
+ tablesListChange.create(name, (tableChange) -> {
+ tableChange.changeDataStorage(
+
dataStorageMgr.defaultTableDataStorageConsumer(tablesChange.defaultDataStorage())
+ );
- var extConfCh = ((ExtendedTableChange) ch);
+ tableInitChange.accept(tableChange);
+
+ var extConfCh = ((ExtendedTableChange) tableChange);
tableCreateFuts.put(extConfCh.id(), tblFut);
// Affinity assignments calculation.
extConfCh.changeAssignments(ByteUtils.toBytes(AffinityUtils.calculateAssignments(
baselineMgr.nodes(),
- ch.partitions(),
- ch.replicas())))
+ tableChange.partitions(),
Review Comment:
As always, you renamed things for no reason :)
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/DataStorageManager.java:
##########
@@ -17,84 +17,104 @@
package org.apache.ignite.internal.storage;
-import static java.util.function.Function.identity;
import static java.util.stream.Collectors.toUnmodifiableMap;
+import static
org.apache.ignite.configuration.schemas.store.DataStorageConfigurationSchema.UNKNOWN_DATA_STORAGE;
+import static org.apache.ignite.internal.util.CollectionUtils.first;
import java.nio.file.Path;
-import java.util.List;
import java.util.Map;
import java.util.ServiceLoader;
+import java.util.function.Consumer;
import java.util.stream.StreamSupport;
+import org.apache.ignite.configuration.schemas.store.DataStorageChange;
import org.apache.ignite.configuration.schemas.store.DataStorageConfiguration;
+import
org.apache.ignite.configuration.schemas.store.DataStorageConfigurationSchema;
+import org.apache.ignite.configuration.schemas.table.TableConfigurationSchema;
+import org.apache.ignite.configuration.schemas.table.TablesConfigurationSchema;
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 {
- /** Mapping: {@link StorageEngine#name} -> {@link StorageEngine}. */
+ /** Mapping: {@link StorageEngineFactory#name} -> {@link StorageEngine}. */
private final Map<String, StorageEngine> engines;
/**
* Constructor.
*
* @param clusterConfigRegistry Register of the (distributed) cluster
configuration.
* @param storagePath Storage path.
- * @throws StorageException If there are duplicates of the data storage
engine.
+ * @param engineFactories Storage engine factories.
+ * @throws IllegalStateException If there are duplicates of the data
storage engine.
*/
public DataStorageManager(
ConfigurationRegistry clusterConfigRegistry,
- Path storagePath
+ Path storagePath,
+ Iterable<StorageEngineFactory> engineFactories
) {
- this.engines = StreamSupport.stream(engineFactories().spliterator(),
false)
- .map(engineFactory ->
engineFactory.createEngine(clusterConfigRegistry, storagePath))
+ engines = StreamSupport.stream(engineFactories.spliterator(), false)
.collect(toUnmodifiableMap(
- StorageEngine::name,
- identity(),
- (storageEngine1, storageEngine2) -> {
- throw new StorageException(String.format(
- "Duplicate key [key=%s, engines=%s]",
- storageEngine1.name(),
- List.of(storageEngine1, storageEngine2)
- ));
- }
+ StorageEngineFactory::name,
+ engineFactory ->
engineFactory.createEngine(clusterConfigRegistry, storagePath)
));
}
+ /**
+ * Constructor, overloads {@link
DataStorageManager#DataStorageManager(ConfigurationRegistry, Path, Iterable)}
with loading factories
+ * through a {@link ServiceLoader}.
+ */
+ public DataStorageManager(
+ ConfigurationRegistry clusterConfigRegistry,
+ Path storagePath
+ ) {
+ this(clusterConfigRegistry, storagePath,
ServiceLoader.load(StorageEngineFactory.class));
+ }
+
/** {@inheritDoc} */
@Override
- public void start() {
+ public void start() throws StorageException {
engines.values().forEach(StorageEngine::start);
}
/** {@inheritDoc} */
@Override
- public void stop() throws Exception {
- IgniteUtils.closeAll(engines.values().stream().map(engine ->
engine::stop));
+ public void stop() throws StorageException {
+ engines.values().forEach(StorageEngine::stop);
}
/**
- * Returns the storage engine factories.
+ * Returns the data storage engine by data storage configuration.
*
- * <p>NOTE: It is recommended to override only in tests.
+ * @param config Data storage configuration.
*/
- protected Iterable<StorageEngineFactory> engineFactories() {
- return ServiceLoader.load(StorageEngineFactory.class);
+ public @Nullable StorageEngine engine(DataStorageConfiguration config) {
+ return engines.get(config.value().name());
}
/**
- * Returns the data storage engine by data storage configuration.
+ * Returns a consumer that will set the default {@link
TableConfigurationSchema#dataStorage table data storage} depending on the {@link
+ * StorageEngine engine}.
*
- * @param config Data storage configuration.
+ * @param defaultDataStorageView View of {@link
TablesConfigurationSchema#defaultDataStorage}. For the case {@link
+ * DataStorageConfigurationSchema#UNKNOWN_DATA_STORAGE} and there is
only one engine, then it will be the default, otherwise there
+ * will be no default.
*/
- public @Nullable StorageEngine engine(DataStorageConfiguration config) {
- return engines.get(config.value().name());
+ public Consumer<DataStorageChange> defaultTableDataStorageConsumer(String
defaultDataStorageView) {
Review Comment:
Ok, if the only purpose of the consumer is to be invoked, then why would you
create it in the first place? Just pass a "change" object here as an argument
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/MockedStructuresTest.java:
##########
@@ -194,13 +199,23 @@ void before() throws NodeStoppingException {
});
};
- dsm = new DataStorageManager(configRegistry, workDir) {
- /** {@inheritDoc} */
- @Override
- protected Iterable<StorageEngineFactory> engineFactories() {
- return List.of((configurationRegistry, storePath) -> new
RocksDbStorageEngine(rocksDbEngineConfig, storePath));
- }
- };
+ dsm = new DataStorageManager(
+ configRegistry,
+ workDir,
+ List.of(new StorageEngineFactory() {
Review Comment:
Can't you instantiate "RocksDbStorageEngineFactory" instead of making
anonymous class?
I believe it would be easy to mock necessary ConfigurationRegistry methods
to provide rocksdb configuration, right?
##########
modules/configuration/src/test/java/org/apache/ignite/internal/configuration/validation/TestValidationUtil.java:
##########
@@ -0,0 +1,99 @@
+/*
+ * 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.configuration.validation;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.empty;
+import static org.hamcrest.Matchers.not;
+import static org.hamcrest.Matchers.startsWith;
+import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.lang.annotation.Annotation;
+import org.apache.ignite.configuration.validation.ValidationContext;
+import org.apache.ignite.configuration.validation.ValidationIssue;
+import org.apache.ignite.configuration.validation.Validator;
+import org.jetbrains.annotations.Nullable;
+import org.mockito.ArgumentCaptor;
+
+/**
+ * Useful class for testing {@link Validator}.
+ */
+public class TestValidationUtil {
+ /**
+ * Creates mock {@link ValidationContext}.
+ *
+ * @param <VIEWT> Type of the subtree or the value that is being validated.
+ * @param oldValue Previous value of the configuration. Might be {@code
null} for leaves only.
+ * @param newValue Updated value of the configuration.
+ */
+ public static <VIEWT> ValidationContext<VIEWT>
mockValidationContext(@Nullable VIEWT oldValue, VIEWT newValue) {
+ ValidationContext<VIEWT> mock = mock(ValidationContext.class);
+
+ when(mock.getOldValue()).thenReturn(oldValue);
+
+ when(mock.getNewValue()).thenReturn(newValue);
+
+ return mock;
+ }
+
+ /**
+ * Adds mocking for method {@link ValidationContext#addIssue}.
+ *
+ * @param <VIEWT> Type of the subtree or the value that is being validated.
+ * @param mock Mocked validation context.
+ * @return New {@link ArgumentCaptor}.
+ */
+ public static <VIEWT> ArgumentCaptor<ValidationIssue>
mockAddIssue(ValidationContext<VIEWT> mock) {
+ ArgumentCaptor<ValidationIssue> issuesCaptor =
ArgumentCaptor.forClass(ValidationIssue.class);
+
+ doNothing().when(mock).addIssue(issuesCaptor.capture());
+
+ return issuesCaptor;
+ }
+
+ /**
+ * Performs validation checking whether there are errors.
+ *
+ * @param validator Checked validator.
+ * @param annotation Mocked annotation.
+ * @param ctx Mocked validation context.
+ * @param errorMessagePrefix Error prefix, if {@code null} it is expected
that there will be no errors.
+ */
+ public static <A extends Annotation, VIEWT> void validate(
+ Validator<A, VIEWT> validator,
+ A annotation,
+ ValidationContext<VIEWT> ctx,
+ @Nullable String errorMessagePrefix
+ ) {
+ ArgumentCaptor<ValidationIssue> argumentCaptor = mockAddIssue(ctx);
+
+ validator.validate(annotation, ctx);
+
+ if (errorMessagePrefix == null) {
+ assertThat(argumentCaptor.getAllValues(), empty());
+ } else {
+ assertThat(argumentCaptor.getAllValues(), not(empty()));
+
+ for (ValidationIssue value : argumentCaptor.getAllValues()) {
Review Comment:
Why do you need to check all errors? Why do they have to be the same?
##########
modules/configuration/src/test/java/org/apache/ignite/internal/configuration/validation/TestValidationUtil.java:
##########
@@ -0,0 +1,99 @@
+/*
+ * 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.configuration.validation;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.empty;
+import static org.hamcrest.Matchers.not;
+import static org.hamcrest.Matchers.startsWith;
+import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.lang.annotation.Annotation;
+import org.apache.ignite.configuration.validation.ValidationContext;
+import org.apache.ignite.configuration.validation.ValidationIssue;
+import org.apache.ignite.configuration.validation.Validator;
+import org.jetbrains.annotations.Nullable;
+import org.mockito.ArgumentCaptor;
+
+/**
+ * Useful class for testing {@link Validator}.
+ */
+public class TestValidationUtil {
+ /**
+ * Creates mock {@link ValidationContext}.
+ *
+ * @param <VIEWT> Type of the subtree or the value that is being validated.
+ * @param oldValue Previous value of the configuration. Might be {@code
null} for leaves only.
+ * @param newValue Updated value of the configuration.
+ */
+ public static <VIEWT> ValidationContext<VIEWT>
mockValidationContext(@Nullable VIEWT oldValue, VIEWT newValue) {
+ ValidationContext<VIEWT> mock = mock(ValidationContext.class);
+
+ when(mock.getOldValue()).thenReturn(oldValue);
+
+ when(mock.getNewValue()).thenReturn(newValue);
+
+ return mock;
+ }
+
+ /**
+ * Adds mocking for method {@link ValidationContext#addIssue}.
+ *
+ * @param <VIEWT> Type of the subtree or the value that is being validated.
+ * @param mock Mocked validation context.
+ * @return New {@link ArgumentCaptor}.
+ */
+ public static <VIEWT> ArgumentCaptor<ValidationIssue>
mockAddIssue(ValidationContext<VIEWT> mock) {
Review Comment:
Not the best name for the method. Can it just be "addIssuesCaptor" or
something?
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/configuration/KnownDataStorageValidator.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.schema.configuration;
+
+import static
org.apache.ignite.configuration.schemas.store.DataStorageConfigurationSchema.UNKNOWN_DATA_STORAGE;
+
+import org.apache.ignite.configuration.schemas.store.DataStorageView;
+import org.apache.ignite.configuration.schemas.store.KnownDataStorage;
+import org.apache.ignite.configuration.validation.ValidationContext;
+import org.apache.ignite.configuration.validation.ValidationIssue;
+import org.apache.ignite.configuration.validation.Validator;
+
+/**
+ * Implementing a validator for {@link KnownDataStorage}.
+ */
+public class KnownDataStorageValidator implements Validator<KnownDataStorage,
DataStorageView> {
+ /** Static instance. */
+ public static final KnownDataStorageValidator INSTANCE = new
KnownDataStorageValidator();
Review Comment:
Dude, why? Why do you need a static instance? If the answer is "because
TableValidatorImpl" did the same - that's a bad answer
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/engine/StorageEngineFactory.java:
##########
@@ -18,16 +18,26 @@
package org.apache.ignite.internal.storage.engine;
import java.nio.file.Path;
+import
org.apache.ignite.configuration.schemas.store.DataStorageConfigurationSchema;
import org.apache.ignite.internal.configuration.ConfigurationRegistry;
import org.apache.ignite.internal.storage.StorageException;
/**
* Factory for creating storage engines.
*/
public interface StorageEngineFactory {
+ /**
+ * Returns the unique name of the storage engine.
+ *
+ * <p>Used to map {@link DataStorageConfigurationSchema#name} to {@link
StorageEngine}.
+ */
+ String name();
+
/**
* Creates a new storage engine.
*
+ * <p>NOTE: {@code configRegistry} has not started yet, so only the root
configuration can be retrieved.
Review Comment:
What does it even mean? Why can't you retrieve other configurations? They
can be accessed, they just empty
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/DataStorageManager.java:
##########
@@ -17,84 +17,104 @@
package org.apache.ignite.internal.storage;
-import static java.util.function.Function.identity;
import static java.util.stream.Collectors.toUnmodifiableMap;
+import static
org.apache.ignite.configuration.schemas.store.DataStorageConfigurationSchema.UNKNOWN_DATA_STORAGE;
+import static org.apache.ignite.internal.util.CollectionUtils.first;
import java.nio.file.Path;
-import java.util.List;
import java.util.Map;
import java.util.ServiceLoader;
+import java.util.function.Consumer;
import java.util.stream.StreamSupport;
+import org.apache.ignite.configuration.schemas.store.DataStorageChange;
import org.apache.ignite.configuration.schemas.store.DataStorageConfiguration;
+import
org.apache.ignite.configuration.schemas.store.DataStorageConfigurationSchema;
+import org.apache.ignite.configuration.schemas.table.TableConfigurationSchema;
+import org.apache.ignite.configuration.schemas.table.TablesConfigurationSchema;
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 {
- /** Mapping: {@link StorageEngine#name} -> {@link StorageEngine}. */
+ /** Mapping: {@link StorageEngineFactory#name} -> {@link StorageEngine}. */
private final Map<String, StorageEngine> engines;
/**
* Constructor.
*
* @param clusterConfigRegistry Register of the (distributed) cluster
configuration.
* @param storagePath Storage path.
- * @throws StorageException If there are duplicates of the data storage
engine.
+ * @param engineFactories Storage engine factories.
+ * @throws IllegalStateException If there are duplicates of the data
storage engine.
*/
public DataStorageManager(
ConfigurationRegistry clusterConfigRegistry,
- Path storagePath
+ Path storagePath,
+ Iterable<StorageEngineFactory> engineFactories
) {
- this.engines = StreamSupport.stream(engineFactories().spliterator(),
false)
- .map(engineFactory ->
engineFactory.createEngine(clusterConfigRegistry, storagePath))
+ engines = StreamSupport.stream(engineFactories.spliterator(), false)
.collect(toUnmodifiableMap(
- StorageEngine::name,
- identity(),
- (storageEngine1, storageEngine2) -> {
- throw new StorageException(String.format(
- "Duplicate key [key=%s, engines=%s]",
- storageEngine1.name(),
- List.of(storageEngine1, storageEngine2)
- ));
- }
+ StorageEngineFactory::name,
+ engineFactory ->
engineFactory.createEngine(clusterConfigRegistry, storagePath)
));
}
+ /**
+ * Constructor, overloads {@link
DataStorageManager#DataStorageManager(ConfigurationRegistry, Path, Iterable)}
with loading factories
+ * through a {@link ServiceLoader}.
+ */
+ public DataStorageManager(
+ ConfigurationRegistry clusterConfigRegistry,
+ Path storagePath
+ ) {
+ this(clusterConfigRegistry, storagePath,
ServiceLoader.load(StorageEngineFactory.class));
+ }
+
/** {@inheritDoc} */
@Override
- public void start() {
+ public void start() throws StorageException {
engines.values().forEach(StorageEngine::start);
}
/** {@inheritDoc} */
@Override
- public void stop() throws Exception {
- IgniteUtils.closeAll(engines.values().stream().map(engine ->
engine::stop));
+ public void stop() throws StorageException {
+ engines.values().forEach(StorageEngine::stop);
Review Comment:
Ok, what was the problem with the old method? New one is worse, can you
guess why?
##########
modules/storage-rocksdb/src/test/java/org/apache/ignite/internal/storage/rocksdb/RocksDbTableStorageTest.java:
##########
@@ -73,21 +75,27 @@ public void setUp(
@InjectConfiguration RocksDbStorageEngineConfiguration
rocksDbEngineConfig,
@InjectConfiguration(
name = "table",
- polymorphicExtensions =
{HashIndexConfigurationSchema.class,
RocksDbDataStorageConfigurationSchema.class}
+ polymorphicExtensions = {
+ HashIndexConfigurationSchema.class,
+ UnknownDataStorageConfigurationSchema.class,
+ RocksDbDataStorageConfigurationSchema.class
+ }
) TableConfiguration tableCfg
) throws Exception {
- assertThat(tableCfg.dataStorage(),
is(instanceOf(RocksDbDataStorageConfiguration.class)));
+ CompletableFuture<Void> changeDataStorageFuture =
tableCfg.dataStorage().change(c -> c.convert(RocksDbDataStorageChange.class));
Review Comment:
In previous test you just wait 1 second, but here you assert that it'll give
you null eventually. Why?
--
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]
