ibessonov commented on code in PR #5816:
URL: https://github.com/apache/ignite-3/pull/5816#discussion_r2097325607
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -645,6 +647,8 @@ private CompletableFuture<Void>
changeInternally0(ConfigurationSource src, long
}
}
+ useAndDeleteLegacyNames(curRoots, localRoots.data.values(),
allChanges);
Review Comment:
I think `dropUnnecessarilyDeletedKeys(allChanges, localRoots);` must be the
last thing that we do, please move it below this line, right above `if
(allChanges.isEmpty() && onStartup) {`.
I didn't notice it last time
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/LegacyNamesTrackingConfigurationVisitor.java:
##########
@@ -0,0 +1,204 @@
+/*
+ * 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.util;
+
+import static
org.apache.ignite.internal.configuration.asm.ConfigurationAsmGenerator.legacyNames;
+import static
org.apache.ignite.internal.configuration.util.ConfigurationUtil.appendKey;
+import static
org.apache.ignite.internal.configuration.util.ConfigurationUtil.join;
+import static org.apache.ignite.internal.util.ArrayUtils.STRING_EMPTY_ARRAY;
+
+import java.io.Serializable;
+import java.lang.reflect.Field;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.function.BiConsumer;
+import org.apache.ignite.internal.configuration.tree.ConfigurationVisitor;
+import org.apache.ignite.internal.configuration.tree.InnerNode;
+import org.apache.ignite.internal.configuration.tree.NamedListNode;
+
+/** Visitor that accumulates keys and legacy names while descending. */
+public abstract class LegacyNamesTrackingConfigurationVisitor<T> implements
ConfigurationVisitor<T> {
Review Comment:
I'll take a look at this class later. Maybe we can delete it actually
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java:
##########
@@ -487,7 +488,20 @@ static String fieldName(Field f) {
public static String publicName(Field f) {
PublicName annotation = f.getAnnotation(PublicName.class);
- return annotation == null ? f.getName() : annotation.value();
+ return annotation == null || "".equals(annotation.value()) ?
f.getName() : annotation.value();
Review Comment:
Could have used `isEmpty()` instead, usually we use `"...".equals(...)` if
the second part is nullable. It's not the case here
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/InnerNodeAsmGenerator.java:
##########
@@ -1188,19 +1189,25 @@ private void addNodeConstructMethod(
);
// this.changePolymorphicTypeId(src == null ? null :
src.unwrap(FieldType.class));
- switchBuilder.addCase(
- publicName,
- new BytecodeBlock()
- .append(constructMtd.getThis())
- .append(getTypeIdFromSrcVar)
- .invokeVirtual(changePolymorphicTypeIdMtd)
- .ret()
- );
+ BytecodeBlock ret = new BytecodeBlock()
Review Comment:
Please come up with a better variable name. And please don't rely too much
on current naming patterns in this class, I don't like them either
##########
modules/runner/src/test/java/org/apache/ignite/internal/configuration/storage/LocalFileConfigurationStorageTest.java:
##########
@@ -575,6 +576,19 @@ void testReadDataOnStartupWithDeletedProperty() throws
IOException {
assertDoesNotThrow(changer::start);
}
+ @Test
+ void testReadDataOnStartupWithRenamedProperty() throws IOException {
+ // Given config in JSON format
+ String fileContent = "top.oldShortValName = 3";
+
+ Path configFile = getConfigFile();
+
+ Files.write(configFile, fileContent.getBytes(StandardCharsets.UTF_8));
+
+ // Storage handles renamed property.
+ assertDoesNotThrow(changer::start);
Review Comment:
But did we read the `3`? I wonder if my tests had this check. Probably not
##########
modules/configuration/src/testFixtures/java/org/apache/ignite/internal/configuration/storage/TestConfigurationStorage.java:
##########
@@ -63,7 +63,7 @@ public TestConfigurationStorage(ConfigurationType type,
Map<String, Serializable
@Override
public void close() {
- // No-op.
+ listeners.clear();
Review Comment:
Is that OK? Please add a comment, like "we reuse this instance after we call
`close`" or something
##########
modules/configuration/src/test/java/org/apache/ignite/internal/configuration/RenamedConfigurationTest.java:
##########
@@ -0,0 +1,279 @@
+/*
+ * 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;
+
+import static
org.apache.ignite.configuration.annotation.ConfigurationType.LOCAL;
+import static
org.apache.ignite.internal.configuration.hocon.HoconConverter.hoconSource;
+import static
org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willBe;
+import static
org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.nullValue;
+
+import com.typesafe.config.ConfigFactory;
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+import org.apache.ignite.configuration.RootKey;
+import org.apache.ignite.configuration.annotation.Config;
+import org.apache.ignite.configuration.annotation.ConfigValue;
+import org.apache.ignite.configuration.annotation.ConfigurationRoot;
+import org.apache.ignite.configuration.annotation.InjectedName;
+import org.apache.ignite.configuration.annotation.NamedConfigValue;
+import org.apache.ignite.configuration.annotation.PolymorphicConfig;
+import org.apache.ignite.configuration.annotation.PolymorphicConfigInstance;
+import org.apache.ignite.configuration.annotation.PolymorphicId;
+import org.apache.ignite.configuration.annotation.PublicName;
+import org.apache.ignite.configuration.annotation.Value;
+import
org.apache.ignite.internal.configuration.storage.TestConfigurationStorage;
+import
org.apache.ignite.internal.configuration.validation.TestConfigurationValidator;
+import org.apache.ignite.internal.manager.ComponentContext;
+import org.apache.ignite.internal.testframework.BaseIgniteAbstractTest;
+import org.hamcrest.Matchers;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+class RenamedConfigurationTest extends BaseIgniteAbstractTest {
+ private static final String OLD_VALUE_NAME = "oldName";
+ private static final String OLD_INNER_NAME = "oldInnerName";
+ private static final String SECOND_OLD_INNER_NAME = "secondOldInnerName";
+ private static final String OLD_LIST_NAME = "listOldName";
+ private static final String POLYMORPHIC_TYPE = "polymorphicType";
+ private static final String OLD_POLYMORPHIC_NAME = "oldPolymorphicName";
+
+ private static final ConfigurationTreeGenerator OLD_GENERATOR = new
ConfigurationTreeGenerator(
+ Set.of(RenamedTestOldConfiguration.KEY),
+ Set.of(),
+ Set.of(RenamedPolymorphicInstanceOldConfigurationSchema.class)
+ );
+
+ private static final ConfigurationTreeGenerator NEW_GENERATOR = new
ConfigurationTreeGenerator(
+ Set.of(RenamedTestNewConfiguration.KEY),
+ Set.of(),
+ Set.of(RenamedPolymorphicInstanceNewConfigurationSchema.class)
+ );
+ public static final String OLD_DEFAULT = "oldDefault";
+
+ private final TestConfigurationStorage storage = new
TestConfigurationStorage(LOCAL);
+
+ private ConfigurationRegistry registry;
+
+ @AfterAll
+ public static void afterAll() {
+ OLD_GENERATOR.close();
+ }
+
+ @BeforeEach
+ void setUp() {
+ registry = startRegistry(RenamedTestOldConfiguration.KEY,
OLD_GENERATOR);
+
+ stopRegistry(registry);
+
+ registry = startRegistry(RenamedTestNewConfiguration.KEY,
NEW_GENERATOR);
+ }
+
+ @AfterEach
+ void tearDown() {
+ stopRegistry(registry);
+ }
+
+ @Test
+ public void testLegacyNameIsRecognisedOnStartup() {
+ // Default value was set when registry was started with old
configuration.
+ assertThat(
+
registry.getConfiguration(RenamedTestNewConfiguration.KEY).newInnerName().newName().value(),
+ Matchers.equalTo(OLD_DEFAULT)
Review Comment:
Please import this method statically
##########
modules/configuration/src/test/java/org/apache/ignite/internal/configuration/RenamedConfigurationTest.java:
##########
@@ -0,0 +1,279 @@
+/*
+ * 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;
+
+import static
org.apache.ignite.configuration.annotation.ConfigurationType.LOCAL;
+import static
org.apache.ignite.internal.configuration.hocon.HoconConverter.hoconSource;
+import static
org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willBe;
+import static
org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.nullValue;
+
+import com.typesafe.config.ConfigFactory;
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+import org.apache.ignite.configuration.RootKey;
+import org.apache.ignite.configuration.annotation.Config;
+import org.apache.ignite.configuration.annotation.ConfigValue;
+import org.apache.ignite.configuration.annotation.ConfigurationRoot;
+import org.apache.ignite.configuration.annotation.InjectedName;
+import org.apache.ignite.configuration.annotation.NamedConfigValue;
+import org.apache.ignite.configuration.annotation.PolymorphicConfig;
+import org.apache.ignite.configuration.annotation.PolymorphicConfigInstance;
+import org.apache.ignite.configuration.annotation.PolymorphicId;
+import org.apache.ignite.configuration.annotation.PublicName;
+import org.apache.ignite.configuration.annotation.Value;
+import
org.apache.ignite.internal.configuration.storage.TestConfigurationStorage;
+import
org.apache.ignite.internal.configuration.validation.TestConfigurationValidator;
+import org.apache.ignite.internal.manager.ComponentContext;
+import org.apache.ignite.internal.testframework.BaseIgniteAbstractTest;
+import org.hamcrest.Matchers;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+class RenamedConfigurationTest extends BaseIgniteAbstractTest {
+ private static final String OLD_VALUE_NAME = "oldName";
+ private static final String OLD_INNER_NAME = "oldInnerName";
+ private static final String SECOND_OLD_INNER_NAME = "secondOldInnerName";
+ private static final String OLD_LIST_NAME = "listOldName";
+ private static final String POLYMORPHIC_TYPE = "polymorphicType";
+ private static final String OLD_POLYMORPHIC_NAME = "oldPolymorphicName";
Review Comment:
Did it improve test readability? =)
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -645,6 +647,8 @@ private CompletableFuture<Void>
changeInternally0(ConfigurationSource src, long
}
}
+ useAndDeleteLegacyNames(curRoots, localRoots.data.values(),
allChanges);
Review Comment:
Why don't we do it inside of `createFlattenedUpdatesMap`? We have to parse
keys that we have just generated right now, looks weird to me personally. Maybe
your code would have been simpler, let's consider this option
##########
modules/configuration/src/test/java/org/apache/ignite/internal/configuration/RenamedConfigurationTest.java:
##########
@@ -0,0 +1,279 @@
+/*
+ * 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;
+
+import static
org.apache.ignite.configuration.annotation.ConfigurationType.LOCAL;
+import static
org.apache.ignite.internal.configuration.hocon.HoconConverter.hoconSource;
+import static
org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willBe;
+import static
org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.nullValue;
+
+import com.typesafe.config.ConfigFactory;
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+import org.apache.ignite.configuration.RootKey;
+import org.apache.ignite.configuration.annotation.Config;
+import org.apache.ignite.configuration.annotation.ConfigValue;
+import org.apache.ignite.configuration.annotation.ConfigurationRoot;
+import org.apache.ignite.configuration.annotation.InjectedName;
+import org.apache.ignite.configuration.annotation.NamedConfigValue;
+import org.apache.ignite.configuration.annotation.PolymorphicConfig;
+import org.apache.ignite.configuration.annotation.PolymorphicConfigInstance;
+import org.apache.ignite.configuration.annotation.PolymorphicId;
+import org.apache.ignite.configuration.annotation.PublicName;
+import org.apache.ignite.configuration.annotation.Value;
+import
org.apache.ignite.internal.configuration.storage.TestConfigurationStorage;
+import
org.apache.ignite.internal.configuration.validation.TestConfigurationValidator;
+import org.apache.ignite.internal.manager.ComponentContext;
+import org.apache.ignite.internal.testframework.BaseIgniteAbstractTest;
+import org.hamcrest.Matchers;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+class RenamedConfigurationTest extends BaseIgniteAbstractTest {
+ private static final String OLD_VALUE_NAME = "oldName";
+ private static final String OLD_INNER_NAME = "oldInnerName";
+ private static final String SECOND_OLD_INNER_NAME = "secondOldInnerName";
+ private static final String OLD_LIST_NAME = "listOldName";
+ private static final String POLYMORPHIC_TYPE = "polymorphicType";
+ private static final String OLD_POLYMORPHIC_NAME = "oldPolymorphicName";
+
+ private static final ConfigurationTreeGenerator OLD_GENERATOR = new
ConfigurationTreeGenerator(
+ Set.of(RenamedTestOldConfiguration.KEY),
+ Set.of(),
+ Set.of(RenamedPolymorphicInstanceOldConfigurationSchema.class)
+ );
+
+ private static final ConfigurationTreeGenerator NEW_GENERATOR = new
ConfigurationTreeGenerator(
+ Set.of(RenamedTestNewConfiguration.KEY),
+ Set.of(),
+ Set.of(RenamedPolymorphicInstanceNewConfigurationSchema.class)
+ );
+ public static final String OLD_DEFAULT = "oldDefault";
+
+ private final TestConfigurationStorage storage = new
TestConfigurationStorage(LOCAL);
+
+ private ConfigurationRegistry registry;
+
+ @AfterAll
+ public static void afterAll() {
+ OLD_GENERATOR.close();
+ }
+
+ @BeforeEach
+ void setUp() {
+ registry = startRegistry(RenamedTestOldConfiguration.KEY,
OLD_GENERATOR);
+
+ stopRegistry(registry);
+
+ registry = startRegistry(RenamedTestNewConfiguration.KEY,
NEW_GENERATOR);
+ }
+
+ @AfterEach
+ void tearDown() {
+ stopRegistry(registry);
+ }
+
+ @Test
+ public void testLegacyNameIsRecognisedOnStartup() {
+ // Default value was set when registry was started with old
configuration.
+ assertThat(
+
registry.getConfiguration(RenamedTestNewConfiguration.KEY).newInnerName().newName().value(),
+ Matchers.equalTo(OLD_DEFAULT)
+ );
+
+ assertThat(storage.readLatest(String.format("key.%s.%s",
OLD_INNER_NAME, OLD_VALUE_NAME)), willBe(nullValue()));
+ }
+
+ @Test
+ public void testLegacyNameIsRecognisedOnUpdate() {
+ String updatedValue = "updatedValue";
+ String configWithFirstLegacyName = String.format("key.%s.%s = \"%s\"",
OLD_INNER_NAME, OLD_VALUE_NAME, updatedValue);
+ updateConfig(registry, configWithFirstLegacyName);
+
+ assertThat(
+
registry.getConfiguration(RenamedTestNewConfiguration.KEY).newInnerName().newName().value(),
+ Matchers.equalTo(updatedValue)
+ );
+ assertThat(storage.readLatest(String.format("key.%s.%s",
OLD_INNER_NAME, OLD_VALUE_NAME)), willBe(nullValue()));
+
+ String secondUpdatedValue = "secondUpdatedValue";
+ String configWithSecondLegacyName = String.format("key.%s.%s =
\"%s\"", SECOND_OLD_INNER_NAME, OLD_VALUE_NAME, secondUpdatedValue);
+
+ updateConfig(registry, configWithSecondLegacyName);
+
+ assertThat(
+
registry.getConfiguration(RenamedTestNewConfiguration.KEY).newInnerName().newName().value(),
+ Matchers.equalTo(secondUpdatedValue)
+ );
+ assertThat(storage.readLatest(String.format("key.%s.%s",
SECOND_OLD_INNER_NAME, OLD_VALUE_NAME)), willBe(nullValue()));
+ }
+
+ @Test
+ public void testNewValuePersistsAfterRestart() {
+ String updatedValue = "updatedValue";
+ String updatedConfig = String.format("key.%s.%s = \"%s\"",
OLD_INNER_NAME, OLD_VALUE_NAME, updatedValue);
+ updateConfig(registry, updatedConfig);
+
+ assertThat(
+
registry.getConfiguration(RenamedTestNewConfiguration.KEY).newInnerName().newName().value(),
+ Matchers.equalTo(updatedValue)
+ );
+
+ stopRegistry(registry);
+ registry = startRegistry(RenamedTestNewConfiguration.KEY,
NEW_GENERATOR);
+
+ assertThat(storage.readLatest(String.format("key.%s.%s",
OLD_INNER_NAME, OLD_VALUE_NAME)), willBe(nullValue()));
+
+ assertThat(
+
registry.getConfiguration(RenamedTestNewConfiguration.KEY).newInnerName().newName().value(),
+ Matchers.equalTo(updatedValue)
+ );
+ }
+
+ @Test
+ public void testNamedListLegacyNameIsRecognisedOnUpdate() {
+ registry = startRegistry(RenamedTestNewConfiguration.KEY,
NEW_GENERATOR);
+
+ String instanceName = "listInstance";
+ String newValue = "newValue";
+
+ String updatedConfig = String.format("key.%s.%s.%s=\"%s\"",
OLD_LIST_NAME, instanceName, OLD_VALUE_NAME, newValue);
Review Comment:
I think that you don't really have to quote the `%s` at the end, let's avoid
all unnecessary noise
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/InnerNodeAsmGenerator.java:
##########
@@ -1188,19 +1189,25 @@ private void addNodeConstructMethod(
);
// this.changePolymorphicTypeId(src == null ? null :
src.unwrap(FieldType.class));
- switchBuilder.addCase(
- publicName,
- new BytecodeBlock()
- .append(constructMtd.getThis())
- .append(getTypeIdFromSrcVar)
- .invokeVirtual(changePolymorphicTypeIdMtd)
- .ret()
- );
+ BytecodeBlock ret = new BytecodeBlock()
+ .append(constructMtd.getThis())
+ .append(getTypeIdFromSrcVar)
+ .invokeVirtual(changePolymorphicTypeIdMtd)
+ .ret();
+
+ addCaseForAllPublicNames(switchBuilder, schemaField, ret);
} else {
switchBuilder.addCase(
publicName,
treatSourceForConstruct(constructMtd, schemaField,
fieldDef).ret()
);
+
Review Comment:
Maybe it's not a good idea to reuse the same `BytecodeBlock` instance in
several `case` clauses, actually. Please check that, it could be your answer
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -694,16 +698,18 @@ private ConfigurationStorageListener
configurationStorageListener() {
return changedEntries -> {
Map<String, ? extends Serializable> changedValues =
changedEntries.values();
- // We need to ignore deletion of deprecated values.
- ignoreDeleted(changedValues, keyIgnorer);
-
StorageRoots oldStorageRoots = storageRoots;
SuperRoot oldSuperRoot = oldStorageRoots.roots;
SuperRoot oldSuperRootNoDefaults =
oldStorageRoots.rootsWithoutDefaults;
SuperRoot newSuperRoot = oldSuperRoot.copy();
SuperRoot newSuperNoDefaults = oldSuperRootNoDefaults.copy();
+ // We need to ignore deletion of deprecated values.
+ ignoreDeleted(changedValues, keyIgnorer);
+ // We need to ignore deletion of legacy values.
+ deleteLegacyKeys(oldStorageRoots, changedValues);
Review Comment:
Actually, that applied to `ignoreDeleted(changedValues, keyIgnorer);` too,
but we won't be refactoring that part right now. I still don't like the usage
of regular expressions, we had to have our own pattern matching that works on
paths instead, the code would be cleaner
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/InnerNodeAsmGenerator.java:
##########
@@ -1216,12 +1223,11 @@ private void addNodeConstructMethod(
}
String fieldName = fieldName(schemaField);
- String publicName = publicName(schemaField);
- switchBuilderAllFields.addCase(
- publicName,
- treatSourceForConstruct(constructMtd, schemaField,
fieldDefs.get(fieldName)).ret()
- );
+ addCaseForAllPublicNames(
+ switchBuilderAllFields,
+ schemaField,
+ treatSourceForConstruct(constructMtd, schemaField,
fieldDefs.get(fieldName)).ret());
Review Comment:
```suggestion
treatSourceForConstruct(constructMtd, schemaField,
fieldDefs.get(fieldName)).ret()
);
```
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -694,16 +698,18 @@ private ConfigurationStorageListener
configurationStorageListener() {
return changedEntries -> {
Map<String, ? extends Serializable> changedValues =
changedEntries.values();
- // We need to ignore deletion of deprecated values.
- ignoreDeleted(changedValues, keyIgnorer);
-
StorageRoots oldStorageRoots = storageRoots;
SuperRoot oldSuperRoot = oldStorageRoots.roots;
SuperRoot oldSuperRootNoDefaults =
oldStorageRoots.rootsWithoutDefaults;
SuperRoot newSuperRoot = oldSuperRoot.copy();
SuperRoot newSuperNoDefaults = oldSuperRootNoDefaults.copy();
+ // We need to ignore deletion of deprecated values.
+ ignoreDeleted(changedValues, keyIgnorer);
+ // We need to ignore deletion of legacy values.
+ deleteLegacyKeys(oldStorageRoots, changedValues);
Review Comment:
I wonder if it would have been simpler to do it in the prefix tree, instead
of raw storage data? Right now you have to parse keys from the storage, while
they will be parsed once again later. You''re doing extra job
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/InnerNodeAsmGenerator.java:
##########
@@ -1188,19 +1189,25 @@ private void addNodeConstructMethod(
);
// this.changePolymorphicTypeId(src == null ? null :
src.unwrap(FieldType.class));
- switchBuilder.addCase(
- publicName,
- new BytecodeBlock()
- .append(constructMtd.getThis())
- .append(getTypeIdFromSrcVar)
- .invokeVirtual(changePolymorphicTypeIdMtd)
- .ret()
- );
+ BytecodeBlock ret = new BytecodeBlock()
+ .append(constructMtd.getThis())
+ .append(getTypeIdFromSrcVar)
+ .invokeVirtual(changePolymorphicTypeIdMtd)
+ .ret();
+
+ addCaseForAllPublicNames(switchBuilder, schemaField, ret);
} else {
switchBuilder.addCase(
publicName,
treatSourceForConstruct(constructMtd, schemaField,
fieldDef).ret()
);
+
Review Comment:
Did you figure it out?
##########
modules/configuration/src/test/java/org/apache/ignite/internal/configuration/RenamedConfigurationTest.java:
##########
@@ -0,0 +1,279 @@
+/*
+ * 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;
+
+import static
org.apache.ignite.configuration.annotation.ConfigurationType.LOCAL;
+import static
org.apache.ignite.internal.configuration.hocon.HoconConverter.hoconSource;
+import static
org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willBe;
+import static
org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.nullValue;
+
+import com.typesafe.config.ConfigFactory;
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+import org.apache.ignite.configuration.RootKey;
+import org.apache.ignite.configuration.annotation.Config;
+import org.apache.ignite.configuration.annotation.ConfigValue;
+import org.apache.ignite.configuration.annotation.ConfigurationRoot;
+import org.apache.ignite.configuration.annotation.InjectedName;
+import org.apache.ignite.configuration.annotation.NamedConfigValue;
+import org.apache.ignite.configuration.annotation.PolymorphicConfig;
+import org.apache.ignite.configuration.annotation.PolymorphicConfigInstance;
+import org.apache.ignite.configuration.annotation.PolymorphicId;
+import org.apache.ignite.configuration.annotation.PublicName;
+import org.apache.ignite.configuration.annotation.Value;
+import
org.apache.ignite.internal.configuration.storage.TestConfigurationStorage;
+import
org.apache.ignite.internal.configuration.validation.TestConfigurationValidator;
+import org.apache.ignite.internal.manager.ComponentContext;
+import org.apache.ignite.internal.testframework.BaseIgniteAbstractTest;
+import org.hamcrest.Matchers;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+class RenamedConfigurationTest extends BaseIgniteAbstractTest {
+ private static final String OLD_VALUE_NAME = "oldName";
+ private static final String OLD_INNER_NAME = "oldInnerName";
+ private static final String SECOND_OLD_INNER_NAME = "secondOldInnerName";
+ private static final String OLD_LIST_NAME = "listOldName";
+ private static final String POLYMORPHIC_TYPE = "polymorphicType";
+ private static final String OLD_POLYMORPHIC_NAME = "oldPolymorphicName";
+
+ private static final ConfigurationTreeGenerator OLD_GENERATOR = new
ConfigurationTreeGenerator(
+ Set.of(RenamedTestOldConfiguration.KEY),
+ Set.of(),
+ Set.of(RenamedPolymorphicInstanceOldConfigurationSchema.class)
+ );
+
+ private static final ConfigurationTreeGenerator NEW_GENERATOR = new
ConfigurationTreeGenerator(
+ Set.of(RenamedTestNewConfiguration.KEY),
+ Set.of(),
+ Set.of(RenamedPolymorphicInstanceNewConfigurationSchema.class)
+ );
+ public static final String OLD_DEFAULT = "oldDefault";
+
+ private final TestConfigurationStorage storage = new
TestConfigurationStorage(LOCAL);
+
+ private ConfigurationRegistry registry;
+
+ @AfterAll
+ public static void afterAll() {
+ OLD_GENERATOR.close();
+ }
+
+ @BeforeEach
+ void setUp() {
+ registry = startRegistry(RenamedTestOldConfiguration.KEY,
OLD_GENERATOR);
+
+ stopRegistry(registry);
+
+ registry = startRegistry(RenamedTestNewConfiguration.KEY,
NEW_GENERATOR);
+ }
+
+ @AfterEach
+ void tearDown() {
+ stopRegistry(registry);
+ }
+
+ @Test
+ public void testLegacyNameIsRecognisedOnStartup() {
+ // Default value was set when registry was started with old
configuration.
+ assertThat(
+
registry.getConfiguration(RenamedTestNewConfiguration.KEY).newInnerName().newName().value(),
+ Matchers.equalTo(OLD_DEFAULT)
+ );
+
+ assertThat(storage.readLatest(String.format("key.%s.%s",
OLD_INNER_NAME, OLD_VALUE_NAME)), willBe(nullValue()));
Review Comment:
Yeah, I'd prefer a hard-coded constant, to be honest with you. But I'm not
insisting
--
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-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
